From 91c3eef11382f5fd5c5546adf159eb6dd304a4d6 Mon Sep 17 00:00:00 2001 From: gngpp Date: Thu, 16 Jul 2026 08:15:03 +0800 Subject: [PATCH 01/10] feat(error): add stable error hierarchy --- Cargo.lock | 1 + Cargo.toml | 1 + examples/error.rb | 12 ++ lib/wreq_ruby/error.rb | 109 ++++++++++++-- lib/wreq_ruby/response.rb | 12 ++ src/client/resp.rs | 26 +++- src/error.rs | 275 ++++++++++++++++++++++------------- src/rt.rs | 4 +- test/error_hierarchy_test.rb | 271 ++++++++++++++++++++++++++++++++++ 9 files changed, 593 insertions(+), 118 deletions(-) create mode 100644 examples/error.rb create mode 100644 test/error_hierarchy_test.rb diff --git a/Cargo.lock b/Cargo.lock index 7c8e22c..f5811fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1545,6 +1545,7 @@ name = "wreq-ruby" version = "1.2.4" dependencies = [ "arc-swap", + "bitflags", "bytes", "cookie", "futures-util", diff --git a/Cargo.toml b/Cargo.toml index bad747a..e38e167 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,7 @@ doc = false name = "wreq_ruby" [dependencies] +bitflags = "2.13.0" magnus = { version = "0.8.2", features = ["bytes"] } rb-sys = { version = "0.9.128", default-features = false } tokio = { version = "1.52.3", features = ["full"] } diff --git a/examples/error.rb b/examples/error.rb new file mode 100644 index 0000000..8a39b69 --- /dev/null +++ b/examples/error.rb @@ -0,0 +1,12 @@ +#!/usr/bin/env ruby + +require_relative "../lib/wreq" + +begin + Wreq.get("not-a-valid-url") +rescue Wreq::Error => error + puts "URI: #{error.uri.inspect}" + puts "Status: #{error.status.inspect}" + puts "#{error.class}: #{error.message}" + puts "Native builder error: #{error.is_builder}" +end diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 745433c..dd2c86e 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -2,10 +2,86 @@ unless defined?(Wreq) module Wreq + # Base class for regular errors raised by wreq-ruby. + # + # This remains a RuntimeError so existing broad rescue handlers continue + # to work. Predicate methods describe a captured native wreq::Error rather + # than the Ruby exception class, and more than one predicate may be true. + # Errors created entirely by the binding return false for every predicate. + class Error < RuntimeError + # The URI associated with the native error. + # + # This explicit accessor may contain credentials or sensitive query + # parameters. Error messages and inspection output omit it, so avoid + # logging this value without redacting it first. + # + # @return [String, nil] frozen URI string, if one was recorded + attr_reader :uri + + # The response status associated with the error. + # + # @return [Integer, nil] HTTP status code, if one was recorded + attr_reader :status + + # @return [Boolean] whether the native error came from a builder + def is_builder + end + + # @return [Boolean] whether the native error came from redirect handling + def is_redirect + end + + # @return [Boolean] whether the native error represents an HTTP status + def is_status + end + + # @return [Boolean] whether the native error is related to a timeout + def is_timeout + end + + # @return [Boolean] whether the native error is related to a request + def is_request + end + + # @return [Boolean] whether the native error is related to connecting + def is_connect + end + + # @return [Boolean] whether the native error is related to proxy connection + def is_proxy_connect + end + + # @return [Boolean] whether the native error is a connection reset + def is_connection_reset + end + + # @return [Boolean] whether the native error is related to a body + def is_body + end + + # @return [Boolean] whether the native error is related to TLS + def is_tls + end + + # @return [Boolean] whether the native error is related to decoding + def is_decode + end + + # @return [Boolean] whether the native error is related to an upgrade + def is_upgrade + end + end + + # Raised when a native request wait is interrupted. + # + # InterruptError stays outside StandardError so broad application rescues + # do not swallow Ruby interrupts. + class InterruptError < Interrupt; end + # System-level and runtime errors # Memory allocation failed. - class MemoryError < StandardError; end + class MemoryError < Error; end # The native extension was inherited from a parent process. # @@ -17,7 +93,7 @@ class MemoryError < StandardError; end # Process.fork do # Wreq::Client.new # Raises when the parent loaded wreq-ruby. # end - class ForkError < RuntimeError; end + class ForkError < Error; end # Network connection errors @@ -32,7 +108,7 @@ class ForkError < RuntimeError; end # puts "Connection failed: #{e.message}" # retry_with_backoff # end - class ConnectionError < StandardError; end + class ConnectionError < Error; end # Proxy Connection to the server failed. # @@ -44,7 +120,7 @@ class ConnectionError < StandardError; end # puts "Proxy connection failed: #{e.message}" # retry_with_different_proxy # end - class ProxyConnectionError < StandardError; end + class ProxyConnectionError < Error; end # Connection was reset by the server. # @@ -54,7 +130,7 @@ class ProxyConnectionError < StandardError; end # rescue Wreq::ConnectionResetError => e # puts "Connection reset: #{e.message}" # end - class ConnectionResetError < StandardError; end + class ConnectionResetError < Error; end # TLS/SSL error occurred. # @@ -67,7 +143,7 @@ class ConnectionResetError < StandardError; end # rescue Wreq::TlsError => e # puts "TLS error: #{e.message}" # end - class TlsError < StandardError; end + class TlsError < Error; end # HTTP protocol and request/response errors @@ -79,20 +155,21 @@ class TlsError < StandardError; end # rescue Wreq::RequestError => e # puts "Request failed: #{e.message}" # end - class RequestError < StandardError; end + class RequestError < Error; end # HTTP status code indicates an error. # - # Raised when the server returns an error status code (4xx or 5xx). + # Raised by Response#raise_for_status! for a 4xx or 5xx response. Requests + # continue to return these responses normally until that method is called. # # @example # begin # response = client.get("https://httpbin.io/status/404") + # response.raise_for_status! # rescue Wreq::StatusError => e - # puts "HTTP error: #{e.message}" - # # e.response contains the full response + # puts "HTTP #{e.status}: #{e.message}" # end - class StatusError < StandardError; end + class StatusError < Error; end # Redirect handling failed. # @@ -105,7 +182,7 @@ class StatusError < StandardError; end # rescue Wreq::RedirectError => e # puts "Too many redirects: #{e.message}" # end - class RedirectError < StandardError; end + class RedirectError < Error; end # Request timed out. # @@ -119,7 +196,7 @@ class RedirectError < StandardError; end # puts "Request timed out: #{e.message}" # retry_with_longer_timeout # end - class TimeoutError < StandardError; end + class TimeoutError < Error; end # Data processing and encoding errors @@ -131,7 +208,7 @@ class TimeoutError < StandardError; end # rescue Wreq::BodyError => e # puts "Body error: #{e.message}" # end - class BodyError < StandardError; end + class BodyError < Error; end # Decoding response failed. # @@ -147,7 +224,7 @@ class BodyError < StandardError; end # # Fall back to binary data # data = response.body # end - class DecodingError < StandardError; end + class DecodingError < Error; end # Configuration and builder errors @@ -162,6 +239,6 @@ class DecodingError < StandardError; end # rescue Wreq::BuilderError => e # puts "Invalid configuration: #{e.message}" # end - class BuilderError < StandardError; end + class BuilderError < Error; end end end diff --git a/lib/wreq_ruby/response.rb b/lib/wreq_ruby/response.rb index fe816fa..d0875ba 100644 --- a/lib/wreq_ruby/response.rb +++ b/lib/wreq_ruby/response.rb @@ -43,6 +43,18 @@ def code def status end + # Raise for a client or server error status. + # + # This check is opt-in and does not consume the response body. + # + # @return [Wreq::Response] This response for non-error statuses + # @raise [Wreq::StatusError] for a 4xx or 5xx status + # @example + # response = client.get("https://example.com/missing") + # response.raise_for_status! + def raise_for_status! + end + # Get the HTTP protocol version used. # # @return [Wreq::Version] HTTP version (HTTP/1.1, HTTP/2, etc.) diff --git a/src/client/resp.rs b/src/client/resp.rs index 10b3811..c814770 100644 --- a/src/client/resp.rs +++ b/src/client/resp.rs @@ -5,7 +5,7 @@ use bytes::Bytes; use futures_util::TryFutureExt; use http::{Extensions, HeaderMap, response::Response as HttpResponse}; use http_body_util::BodyExt; -use magnus::{Error, Module, RArray, RModule, Ruby, Value, scan_args::scan_args}; +use magnus::{Error, Module, RArray, RModule, Ruby, Value, scan_args::scan_args, typed_data::Obj}; use wreq::Uri; use crate::{ @@ -63,6 +63,14 @@ impl Response { } } + /// Build a body-free native response for its status classification logic. + fn response_for_status(&self) -> wreq::Response { + let mut response = HttpResponse::new(Bytes::new()); + *response.status_mut() = self.status.0; + *response.extensions_mut() = self.extensions.clone(); + wreq::Response::from(response) + } + /// Internal method to get the wreq::Response, optionally streaming the body. fn response(&self, ruby: &Ruby, stream: bool) -> Result { rt::ensure_current(ruby)?; @@ -123,6 +131,18 @@ impl Response { self.status } + /// Return this response unless its status is a client or server error. + /// + /// Delegates classification to `wreq::Response::error_for_status_ref` + /// without consuming the response body. + pub fn raise_for_status(ruby: &Ruby, rb_self: Obj) -> Result, Error> { + rb_self + .response_for_status() + .error_for_status_ref() + .map_err(|err| wreq_error(ruby, err))?; + Ok(rb_self) + } + /// Get the response HTTP version. #[inline] pub fn version(&self) -> Version { @@ -230,6 +250,10 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), Error> { let response = gem_module.define_class("Response", ruby.class_object())?; response.define_method("code", magnus::method!(Response::code, 0))?; response.define_method("status", magnus::method!(Response::status, 0))?; + response.define_method( + "raise_for_status!", + magnus::method!(Response::raise_for_status, 0), + )?; response.define_method("version", magnus::method!(Response::version, 0))?; response.define_method("url", magnus::method!(Response::url, 0))?; response.define_method( diff --git a/src/error.rs b/src/error.rs index 81a8ed7..9bbb75e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -3,12 +3,15 @@ use std::{ fmt, }; +use bitflags::bitflags; use magnus::{ - Error as MagnusError, RModule, Ruby, error::ErrorType, exception::ExceptionClass, prelude::*, - value::Lazy, + Attr, Class, Error as MagnusError, Exception, RModule, RObject, Ruby, TryConvert, + error::ErrorType, exception::ExceptionClass, prelude::*, value::Lazy, }; use tokio::sync::mpsc::error::SendError; +const ERROR_FLAGS_IVAR: &str = "wreq_error_flags"; + const RACE_CONDITION_ERROR_MSG: &str = r#"Due to Rust's memory management with borrowing, you cannot use certain instances multiple times as they may be consumed. @@ -35,25 +38,74 @@ macro_rules! define_exception { } macro_rules! initialize_exception { - ($ruby:expr, $module:expr, $name:ident, $ruby_name:literal, $parent_method:ident) => {{ - $module.define_error($ruby_name, $ruby.$parent_method())?; + ($ruby:expr, $module:expr, $name:ident, $ruby_name:literal, $parent:expr) => {{ + $module.define_error($ruby_name, $parent)?; Lazy::force(&$name, $ruby); }}; } -macro_rules! map_wreq_error { - ($ruby:expr, $err:expr, $msg:expr, $($check_method:ident => $exception:ident),* $(,)?) => { - { +macro_rules! define_error_predicates { + ($($method:ident => $flag:ident = $value:expr),+ $(,)?) => { + bitflags! { + /// Native predicates retained after consuming a wreq error. + struct ErrorFlags: u16 { + $(const $flag = $value;)+ + } + } + + $( + fn $method(rb_self: RObject) -> Result { + error_has_flag(rb_self, ErrorFlags::$flag) + } + )+ + + /// Snapshot every native predicate before consuming the wreq error. + fn wreq_error_flags(err: &wreq::Error) -> ErrorFlags { + let mut flags = ErrorFlags::empty(); $( - if $err.$check_method() { - return MagnusError::new($ruby.get_inner(&$exception), $msg); + if err.$method() { + flags.insert(ErrorFlags::$flag); } - )* - MagnusError::new($ruby.exception_runtime_error(), $msg) + )+ + flags + } + + /// Define the native wreq predicate methods on `Wreq::Error`. + fn include_error_predicates(class: ExceptionClass) -> Result<(), MagnusError> { + $( + class.define_method(stringify!($method), magnus::method!($method, 0))?; + )+ + Ok(()) } }; } +define_error_predicates! { + is_builder => IS_BUILDER = 1 << 0, + is_redirect => IS_REDIRECT = 1 << 1, + is_status => IS_STATUS = 1 << 2, + is_timeout => IS_TIMEOUT = 1 << 3, + is_request => IS_REQUEST = 1 << 4, + is_connect => IS_CONNECT = 1 << 5, + is_proxy_connect => IS_PROXY_CONNECT = 1 << 6, + is_connection_reset => IS_CONNECTION_RESET = 1 << 7, + is_body => IS_BODY = 1 << 8, + is_tls => IS_TLS = 1 << 9, + is_decode => IS_DECODE = 1 << 10, + is_upgrade => IS_UPGRADE = 1 << 11, +} + +/// Native error details retained after converting a wreq error to Ruby. +struct ErrorMetadata<'a> { + uri: Option<&'a str>, + status: Option, + flags: ErrorFlags, +} + +// Stable roots for native errors. +define_exception!(WREQ_ERROR, "Error", exception_runtime_error); +define_exception!(INTERRUPT_ERROR, "InterruptError", exception_interrupt); + // System-level and runtime errors define_exception!(MEMORY, "MemoryError", exception_runtime_error); define_exception!(FORK_ERROR, "ForkError", exception_runtime_error); @@ -90,9 +142,9 @@ pub fn memory_error(ruby: &Ruby) -> MagnusError { MagnusError::new(ruby.get_inner(&MEMORY), RACE_CONDITION_ERROR_MSG) } -/// Create Ruby's standard thread interruption error. +/// Create a `Wreq::InterruptError` outside the `StandardError` hierarchy. pub fn interrupt_error(ruby: &Ruby) -> MagnusError { - MagnusError::new(ruby.exception_interrupt(), "request interrupted") + MagnusError::new(ruby.get_inner(&INTERRUPT_ERROR), "request interrupted") } /// Build `Wreq::ForkError` without touching inherited native state. @@ -186,19 +238,17 @@ pub fn json_serialization_error(ruby: &Ruby, err: MagnusError) -> MagnusError { ) } -/// Prefix a Magnus error while preserving its original Ruby exception class. -pub(crate) fn contextualize_magnus_error( - err: MagnusError, - context: fmt::Arguments<'_>, -) -> MagnusError { +/// Prefix a Magnus error while preserving its Ruby exception class and cause. +pub fn contextualize_magnus_error(err: MagnusError, context: fmt::Arguments<'_>) -> MagnusError { match err.error_type() { ErrorType::Error(class, message) => { MagnusError::new(*class, format!("{context}: {message}")) } - ErrorType::Exception(exception) => MagnusError::new( - exception.exception_class(), - format!("{context}: {exception}"), - ), + ErrorType::Exception(exception) => { + let contextualized: Result = + exception.funcall("exception", (format!("{context}: {exception}"),)); + contextualized.map_or_else(|error| error, MagnusError::from) + } ErrorType::Jump(_) => err, } } @@ -222,24 +272,89 @@ pub fn range_error(ruby: &Ruby, message: impl Into) -> MagnusError { pub fn type_error(ruby: &Ruby, message: impl Into) -> MagnusError { MagnusError::new(ruby.exception_type_error(), message.into()) } -/// Map [`wreq::Error`] to corresponding [`magnus::Error`] + +/// Select the most specific Ruby exception class for native predicate flags. +fn wreq_error_class(ruby: &Ruby, flags: &ErrorFlags) -> ExceptionClass { + let class = if flags.contains(ErrorFlags::IS_BUILDER) { + &BUILDER_ERROR + } else if flags.contains(ErrorFlags::IS_BODY) { + &BODY_ERROR + } else if flags.contains(ErrorFlags::IS_TLS) { + &TLS_ERROR + } else if flags.contains(ErrorFlags::IS_CONNECTION_RESET) { + &CONNECTION_RESET_ERROR + } else if flags.contains(ErrorFlags::IS_CONNECT) { + &CONNECTION_ERROR + } else if flags.contains(ErrorFlags::IS_PROXY_CONNECT) { + &PROXY_CONNECTION_ERROR + } else if flags.contains(ErrorFlags::IS_DECODE) { + &DECODING_ERROR + } else if flags.contains(ErrorFlags::IS_REDIRECT) { + &REDIRECT_ERROR + } else if flags.contains(ErrorFlags::IS_TIMEOUT) { + &TIMEOUT_ERROR + } else if flags.contains(ErrorFlags::IS_STATUS) { + &STATUS_ERROR + } else if flags.contains(ErrorFlags::IS_REQUEST) { + &REQUEST_ERROR + } else { + &WREQ_ERROR + }; + ruby.get_inner(class) +} + +/// Read one native predicate from a Ruby error, defaulting to `false`. +fn error_has_flag(rb_self: RObject, flag: ErrorFlags) -> Result { + rb_self + .ivar_get::<_, Option>(ERROR_FLAGS_IVAR) + .map(|flags| flags.is_some_and(|flags| ErrorFlags::from_bits_retain(flags).contains(flag))) +} + +/// Construct a Ruby exception and attach immutable native error metadata. +fn error_with_metadata( + ruby: &Ruby, + class: ExceptionClass, + message: String, + metadata: ErrorMetadata<'_>, +) -> MagnusError { + match class.new_instance((message,)).and_then(|exception| { + let object = RObject::try_convert(exception.as_value())?; + object.ivar_set(ERROR_FLAGS_IVAR, metadata.flags.bits())?; + + if let Some(uri) = metadata.uri { + let uri = ruby.str_new(uri); + uri.freeze(); + object.ivar_set("@uri", uri)?; + } + + if let Some(status) = metadata.status { + object.ivar_set("@status", status.as_u16())?; + } + + Ok(exception) + }) { + Ok(exception) => exception.into(), + Err(error) => error, + } +} + +/// Map [`wreq::Error`] to corresponding [`magnus::Error`]. pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { - let error_msg = err.to_string(); - map_wreq_error!( + let flags = wreq_error_flags(&err); + let class = wreq_error_class(ruby, &flags); + let uri = err.uri().map(ToString::to_string); + let status = err.status(); + let message = err.without_uri().to_string(); + + error_with_metadata( ruby, - err, - error_msg, - is_builder => BUILDER_ERROR, - is_body => BODY_ERROR, - is_tls => TLS_ERROR, - is_connection_reset => CONNECTION_RESET_ERROR, - is_connect => CONNECTION_ERROR, - is_proxy_connect => PROXY_CONNECTION_ERROR, - is_decode => DECODING_ERROR, - is_redirect => REDIRECT_ERROR, - is_timeout => TIMEOUT_ERROR, - is_status => STATUS_ERROR, - is_request => REQUEST_ERROR, + class, + message, + ErrorMetadata { + uri: uri.as_deref(), + status, + flags, + }, ) } @@ -249,96 +364,58 @@ pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { /// /// Returns the Ruby exception raised while defining an error class. pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), MagnusError> { - initialize_exception!( - ruby, - gem_module, - MEMORY, - "MemoryError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - FORK_ERROR, - "ForkError", - exception_runtime_error - ); + let error_class = gem_module.define_error("Error", ruby.exception_runtime_error())?; + error_class.define_attr("uri", Attr::Read)?; + error_class.define_attr("status", Attr::Read)?; + include_error_predicates(error_class)?; + Lazy::force(&WREQ_ERROR, ruby); + + gem_module.define_error("InterruptError", ruby.exception_interrupt())?; + Lazy::force(&INTERRUPT_ERROR, ruby); + + initialize_exception!(ruby, gem_module, MEMORY, "MemoryError", error_class); + initialize_exception!(ruby, gem_module, FORK_ERROR, "ForkError", error_class); initialize_exception!( ruby, gem_module, CONNECTION_ERROR, "ConnectionError", - exception_runtime_error + error_class ); initialize_exception!( ruby, gem_module, PROXY_CONNECTION_ERROR, "ProxyConnectionError", - exception_runtime_error + error_class ); initialize_exception!( ruby, gem_module, CONNECTION_RESET_ERROR, "ConnectionResetError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - TLS_ERROR, - "TlsError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - REQUEST_ERROR, - "RequestError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - STATUS_ERROR, - "StatusError", - exception_runtime_error + error_class ); + initialize_exception!(ruby, gem_module, TLS_ERROR, "TlsError", error_class); + initialize_exception!(ruby, gem_module, REQUEST_ERROR, "RequestError", error_class); + + initialize_exception!(ruby, gem_module, STATUS_ERROR, "StatusError", error_class); initialize_exception!( ruby, gem_module, REDIRECT_ERROR, "RedirectError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - TIMEOUT_ERROR, - "TimeoutError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - BODY_ERROR, - "BodyError", - exception_runtime_error + error_class ); + initialize_exception!(ruby, gem_module, TIMEOUT_ERROR, "TimeoutError", error_class); + initialize_exception!(ruby, gem_module, BODY_ERROR, "BodyError", error_class); initialize_exception!( ruby, gem_module, DECODING_ERROR, "DecodingError", - exception_runtime_error - ); - initialize_exception!( - ruby, - gem_module, - BUILDER_ERROR, - "BuilderError", - exception_runtime_error + error_class ); + initialize_exception!(ruby, gem_module, BUILDER_ERROR, "BuilderError", error_class); Ok(()) } diff --git a/src/rt.rs b/src/rt.rs index c037401..a385f37 100644 --- a/src/rt.rs +++ b/src/rt.rs @@ -60,8 +60,8 @@ pub fn ensure_current(ruby: &Ruby) -> Result<(), magnus::Error> { /// # Errors /// /// Returns `Wreq::ForkError` if the extension belongs to a parent process, -/// `Wreq::BuilderError` if the Tokio runtime cannot be initialized, Ruby's -/// standard `Interrupt` if Ruby interrupts the request, or the error produced +/// `Wreq::BuilderError` if the Tokio runtime cannot be initialized, +/// `Wreq::InterruptError` if Ruby interrupts the request, or the error produced /// by `map_err` if the future fails. pub fn try_block_on(ruby: &Ruby, future: F, map_err: M) -> Result where diff --git a/test/error_hierarchy_test.rb b/test/error_hierarchy_test.rb new file mode 100644 index 0000000..b7c1fe9 --- /dev/null +++ b/test/error_hierarchy_test.rb @@ -0,0 +1,271 @@ +require "test_helper" +require "socket" +require "timeout" + +class ErrorHierarchyTest < Minitest::Test + REGULAR_ERROR_NAMES = %i[ + MemoryError + ForkError + ConnectionError + ProxyConnectionError + ConnectionResetError + TlsError + RequestError + StatusError + RedirectError + TimeoutError + BodyError + DecodingError + BuilderError + ].freeze + NATIVE_ERROR_PREDICATES = %i[ + is_builder + is_redirect + is_status + is_timeout + is_request + is_connect + is_proxy_connect + is_connection_reset + is_body + is_tls + is_decode + is_upgrade + ].freeze + + def test_regular_errors_share_stable_root + assert_equal RuntimeError, Wreq::Error.superclass + assert_operator Wreq::Error, :<, StandardError + + REGULAR_ERROR_NAMES.each do |name| + assert_equal Wreq::Error, Wreq.const_get(name).superclass + end + + error = Wreq::MemoryError.new + assert_nil error.uri + assert_nil error.status + NATIVE_ERROR_PREDICATES.each do |predicate| + assert_equal false, error.public_send(predicate) + end + end + + def test_root_and_specific_errors_can_be_rescued + root_error = begin + Wreq.get("not-a-valid-url") + rescue Wreq::Error => error + error + end + + assert_instance_of Wreq::BuilderError, root_error + assert root_error.is_builder + assert_nil root_error.status + assert_equal [:is_builder], active_native_predicates(root_error) + assert_raises(Wreq::BuilderError) { Wreq.get("not-a-valid-url") } + end + + def test_binding_generated_errors_have_no_native_predicates + error = assert_raises(Wreq::BuilderError) { Wreq::Headers.new(Object.new) } + + assert_empty active_native_predicates(error) + end + + def test_native_error_predicates_are_not_mutually_exclusive + with_hanging_server do |url, _accepted| + error = assert_raises(Wreq::TimeoutError) { Wreq.get(url, timeout: 1) } + + assert error.is_timeout + assert error.is_request + end + end + + def test_interrupt_error_stays_outside_standard_error + assert_equal Interrupt, Wreq::InterruptError.superclass + refute_operator Wreq::InterruptError, :<, StandardError + + error = assert_raises(Interrupt) do + raise Wreq::InterruptError, "request interrupted" + end + assert_instance_of Wreq::InterruptError, error + end + + def test_request_interruption_raises_interrupt_error + request_thread = nil + with_hanging_server do |url, accepted| + request_thread = Thread.new do + Wreq.get(url, timeout: 60) + rescue Interrupt, StandardError => error + error + end + request_thread.report_on_exception = false + + Timeout.timeout(5) { accepted.pop } + request_thread.wakeup + + assert request_thread.join(5), "Interrupted request thread should stop" + + error = request_thread.value + assert_instance_of Wreq::InterruptError, error + refute_kind_of StandardError, error + end + ensure + request_thread&.kill + request_thread&.join(1) + end + + def test_raise_for_status_returns_same_successful_response + with_status_server(204) do |url| + response = Wreq.get(url) + + assert_same response, response.raise_for_status! + end + end + + def test_raise_for_status_exposes_status_without_consuming_body + {404 => "missing", 503 => "unavailable"}.each do |status, body| + with_status_server(status, body:) do |url| + response = Wreq.get("#{url}?token=response-secret#fragment-secret") + error = assert_raises(Wreq::StatusError) { response.raise_for_status! } + + assert_kind_of Wreq::Error, error + assert_instance_of Integer, error.status + assert_equal status, error.status + assert_equal response.url, error.uri + assert_predicate error.uri, :frozen? + assert error.is_status + assert_equal [:is_status], active_native_predicates(error) + refute_respond_to error, :kind + refute_respond_to error, :retryable? + refute_includes error.message, "response-secret" + refute_includes error.inspect, "response-secret" + assert_equal body, response.text + end + end + end + + def test_native_error_messages_hide_sensitive_request_data + port = closed_local_port + error = assert_raises(Wreq::Error) do + Wreq.get( + "http://uri-user:uri-password@127.0.0.1:#{port}/private?token=query-secret#fragment-secret", + proxy: "http://proxy-user:proxy-secret@127.0.0.1:#{port}", + headers: {"Authorization" => "Bearer authorization-secret"}, + cookies: {"session" => "cookie-secret"}, + timeout: 1 + ) + end + + assert_instance_of String, error.uri + assert_predicate error.uri, :frozen? + assert_includes error.uri, "query-secret" + assert NATIVE_ERROR_PREDICATES.any? { |predicate| error.public_send(predicate) } + assert_nil error.status + NATIVE_ERROR_PREDICATES.each do |predicate| + assert_includes [true, false], error.public_send(predicate) + end + + [ + "uri-user", + "uri-password", + "query-secret", + "fragment-secret", + "proxy-user", + "proxy-secret", + "authorization-secret", + "cookie-secret" + ].each do |secret| + refute_includes error.message, secret + refute_includes error.inspect, secret + end + end + + def test_option_conversion_preserves_exception_cause + source = Object.new + source.define_singleton_method(:to_a) do + raise ArgumentError, "original conversion failure" + rescue ArgumentError => cause + raise IOError, "header conversion failed", cause: cause + end + + error = assert_raises(IOError) { Wreq::Client.new(headers: source) } + + assert_includes error.message, ":headers" + assert_instance_of ArgumentError, error.cause + assert_equal "original conversion failure", error.cause.message + end + + def test_option_context_preserves_native_error_metadata + error = assert_raises(Wreq::BuilderError) { Wreq::Client.new(proxy: "://") } + + assert_includes error.message, ":proxy" + assert_equal [:is_builder], active_native_predicates(error) + end + + private + + def active_native_predicates(error) + NATIVE_ERROR_PREDICATES.select { |predicate| error.public_send(predicate) } + end + + def closed_local_port + server = TCPServer.new("127.0.0.1", 0) + server.addr[1] + ensure + server&.close + end + + def with_hanging_server + server = TCPServer.new("127.0.0.1", 0) + accepted = Queue.new + thread = Thread.new do + socket = server.accept + accepted << true + sleep + rescue IOError, SystemCallError + nil + ensure + socket&.close unless socket&.closed? + end + thread.report_on_exception = false + + yield "http://127.0.0.1:#{server.addr[1]}/", accepted + ensure + server&.close unless server&.closed? + thread&.kill + thread&.join(1) + end + + def with_status_server(status, body: "") + reason = { + 204 => "No Content", + 404 => "Not Found", + 503 => "Service Unavailable" + }.fetch(status) + server = TCPServer.new("127.0.0.1", 0) + port = server.addr[1] + thread = Thread.new do + socket = server.accept + begin + while (line = socket.gets) + break if line == "\r\n" + end + socket.write "HTTP/1.1 #{status} #{reason}\r\n" + socket.write "Content-Type: text/plain\r\n" + socket.write "Content-Length: #{body.bytesize}\r\n" + socket.write "Connection: close\r\n\r\n" + socket.write body + ensure + socket.close unless socket.closed? + end + rescue IOError, SystemCallError + nil + ensure + server.close unless server.closed? + end + thread.report_on_exception = false + + yield "http://127.0.0.1:#{port}/" + ensure + server&.close unless server&.closed? + thread&.join(5) + end +end From f9a224be9c59ad30c9f148889bb09864dcded591 Mon Sep 17 00:00:00 2001 From: gngpp Date: Thu, 16 Jul 2026 19:13:38 +0800 Subject: [PATCH 02/10] Refactor wreq error classification metadata --- Cargo.lock | 1 - Cargo.toml | 1 - src/error.rs | 307 +++++++++++++++++++++++++++------------------------ 3 files changed, 161 insertions(+), 148 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fec960a..3c90168 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1554,7 +1554,6 @@ name = "wreq-ruby" version = "1.2.4" dependencies = [ "arc-swap", - "bitflags", "bytes", "cookie", "forkguard", diff --git a/Cargo.toml b/Cargo.toml index 360efc3..db49e7c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,7 +14,6 @@ doc = false name = "wreq_ruby" [dependencies] -bitflags = "2.13.0" magnus = { version = "0.8.2", features = ["bytes"] } rb-sys = { version = "0.9.128", default-features = false } tokio = { version = "1.52.3", features = ["full"] } diff --git a/src/error.rs b/src/error.rs index 9bbb75e..d5da40f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,16 +1,16 @@ use std::{ + borrow::Cow, cell::{BorrowError, BorrowMutError}, fmt, }; -use bitflags::bitflags; use magnus::{ Attr, Class, Error as MagnusError, Exception, RModule, RObject, Ruby, TryConvert, error::ErrorType, exception::ExceptionClass, prelude::*, value::Lazy, }; use tokio::sync::mpsc::error::SendError; -const ERROR_FLAGS_IVAR: &str = "wreq_error_flags"; +const ERROR_PREDICATES_IVAR: &str = "wreq_error_predicates"; const RACE_CONDITION_ERROR_MSG: &str = r#"Due to Rust's memory management with borrowing, you cannot use certain instances multiple times as they may be consumed. @@ -44,62 +44,138 @@ macro_rules! initialize_exception { }}; } -macro_rules! define_error_predicates { - ($($method:ident => $flag:ident = $value:expr),+ $(,)?) => { - bitflags! { - /// Native predicates retained after consuming a wreq error. - struct ErrorFlags: u16 { - $(const $flag = $value;)+ +macro_rules! define_error_mapping { + ($($predicate:ident: $method:ident => $class:ident $(($ruby_name:literal))?),+ $(,)?) => { + /// Native predicates ordered by Ruby exception class priority. + #[derive(Clone, Copy)] + #[repr(u8)] + enum ErrorPredicate { + $($predicate),+ + } + + impl ErrorPredicate { + const CLASSIFICATION_ORDER: &'static [Self] = &[$(Self::$predicate),+]; + + /// Return this predicate's position in the compact Ruby metadata. + const fn mask(self) -> u16 { + 1_u16 << (self as u8) + } + + /// Evaluate this predicate before the native error is consumed. + fn matches_wreq(self, error: &wreq::Error) -> bool { + match self { + $(Self::$predicate => error.$method(),)+ + } + } + + /// Return the Ruby class selected when this predicate has priority. + fn error_class(self) -> &'static Lazy { + match self { + $(Self::$predicate => &$class,)+ + } } } + const _: () = + assert!(ErrorPredicate::CLASSIFICATION_ORDER.len() <= u16::BITS as usize); + + $( + $(define_exception!($class, $ruby_name, exception_runtime_error);)? + )+ + $( fn $method(rb_self: RObject) -> Result { - error_has_flag(rb_self, ErrorFlags::$flag) + error_has_predicate(rb_self, ErrorPredicate::$predicate) } )+ - /// Snapshot every native predicate before consuming the wreq error. - fn wreq_error_flags(err: &wreq::Error) -> ErrorFlags { - let mut flags = ErrorFlags::empty(); + /// Define the native wreq predicate methods on Wreq::Error. + fn include_error_predicates(class: ExceptionClass) -> Result<(), MagnusError> { $( - if err.$method() { - flags.insert(ErrorFlags::$flag); - } + class.define_method(stringify!($method), magnus::method!($method, 0))?; )+ - flags + Ok(()) } - /// Define the native wreq predicate methods on `Wreq::Error`. - fn include_error_predicates(class: ExceptionClass) -> Result<(), MagnusError> { + /// Define and retain every mapped Ruby exception class. + fn initialize_mapped_errors( + ruby: &Ruby, + gem_module: &RModule, + parent: ExceptionClass, + ) -> Result<(), MagnusError> { $( - class.define_method(stringify!($method), magnus::method!($method, 0))?; + $( + initialize_exception!(ruby, gem_module, $class, $ruby_name, parent); + )? )+ Ok(()) } }; } -define_error_predicates! { - is_builder => IS_BUILDER = 1 << 0, - is_redirect => IS_REDIRECT = 1 << 1, - is_status => IS_STATUS = 1 << 2, - is_timeout => IS_TIMEOUT = 1 << 3, - is_request => IS_REQUEST = 1 << 4, - is_connect => IS_CONNECT = 1 << 5, - is_proxy_connect => IS_PROXY_CONNECT = 1 << 6, - is_connection_reset => IS_CONNECTION_RESET = 1 << 7, - is_body => IS_BODY = 1 << 8, - is_tls => IS_TLS = 1 << 9, - is_decode => IS_DECODE = 1 << 10, - is_upgrade => IS_UPGRADE = 1 << 11, +// The first matching entry determines the Ruby exception class. +define_error_mapping! { + Builder: is_builder => BUILDER_ERROR("BuilderError"), + Body: is_body => BODY_ERROR("BodyError"), + Tls: is_tls => TLS_ERROR("TlsError"), + ConnectionReset: is_connection_reset => CONNECTION_RESET_ERROR("ConnectionResetError"), + Connect: is_connect => CONNECTION_ERROR("ConnectionError"), + ProxyConnect: is_proxy_connect => PROXY_CONNECTION_ERROR("ProxyConnectionError"), + Decode: is_decode => DECODING_ERROR("DecodingError"), + Redirect: is_redirect => REDIRECT_ERROR("RedirectError"), + Timeout: is_timeout => TIMEOUT_ERROR("TimeoutError"), + Status: is_status => STATUS_ERROR("StatusError"), + Request: is_request => REQUEST_ERROR("RequestError"), + Upgrade: is_upgrade => WREQ_ERROR, +} + +/// Native predicates retained after consuming a wreq error. +#[derive(Clone, Copy, Default)] +struct ErrorPredicates(u16); + +impl ErrorPredicates { + /// Restore predicates from compact Ruby metadata. + const fn from_bits(bits: u16) -> Self { + Self(bits) + } + + /// Return the compact representation stored on the Ruby exception. + const fn bits(self) -> u16 { + self.0 + } + + /// Return whether the set contains a native predicate. + const fn contains(self, predicate: ErrorPredicate) -> bool { + self.0 & predicate.mask() != 0 + } + + /// Include a predicate when its native check succeeds. + #[must_use] + const fn include_if(mut self, predicate: ErrorPredicate, include: bool) -> Self { + if include { + self.0 |= predicate.mask(); + } + self + } +} + +impl From<&wreq::Error> for ErrorPredicates { + /// Snapshot every native predicate before consuming the wreq error. + fn from(error: &wreq::Error) -> Self { + ErrorPredicate::CLASSIFICATION_ORDER + .iter() + .copied() + .fold(Self::default(), |predicates, predicate| { + predicates.include_if(predicate, predicate.matches_wreq(error)) + }) + } } /// Native error details retained after converting a wreq error to Ruby. struct ErrorMetadata<'a> { uri: Option<&'a str>, status: Option, - flags: ErrorFlags, + predicates: ErrorPredicates, } // Stable roots for native errors. @@ -110,33 +186,6 @@ define_exception!(INTERRUPT_ERROR, "InterruptError", exception_interrupt); define_exception!(MEMORY, "MemoryError", exception_runtime_error); define_exception!(FORK_ERROR, "ForkError", exception_runtime_error); -// Network connection errors -define_exception!(CONNECTION_ERROR, "ConnectionError", exception_runtime_error); -define_exception!( - PROXY_CONNECTION_ERROR, - "ProxyConnectionError", - exception_runtime_error -); -define_exception!( - CONNECTION_RESET_ERROR, - "ConnectionResetError", - exception_runtime_error -); -define_exception!(TLS_ERROR, "TlsError", exception_runtime_error); - -// HTTP protocol and request/response errors -define_exception!(REQUEST_ERROR, "RequestError", exception_runtime_error); -define_exception!(STATUS_ERROR, "StatusError", exception_runtime_error); -define_exception!(REDIRECT_ERROR, "RedirectError", exception_runtime_error); -define_exception!(TIMEOUT_ERROR, "TimeoutError", exception_runtime_error); - -// Data processing and encoding errors -define_exception!(BODY_ERROR, "BodyError", exception_runtime_error); -define_exception!(DECODING_ERROR, "DecodingError", exception_runtime_error); - -// Configuration and builder errors -define_exception!(BUILDER_ERROR, "BuilderError", exception_runtime_error); - /// Memory error constant pub fn memory_error(ruby: &Ruby) -> MagnusError { MagnusError::new(ruby.get_inner(&MEMORY), RACE_CONDITION_ERROR_MSG) @@ -259,55 +308,36 @@ pub fn option_value_error(option: &str, err: MagnusError) -> MagnusError { } /// Build an `ArgumentError` from a validation message. -pub fn argument_error(ruby: &Ruby, message: impl Into) -> MagnusError { - MagnusError::new(ruby.exception_arg_error(), message.into()) +pub fn argument_error(ruby: &Ruby, message: impl Into>) -> MagnusError { + MagnusError::new(ruby.exception_arg_error(), message) } /// Build a `RangeError` from a validation message. -pub fn range_error(ruby: &Ruby, message: impl Into) -> MagnusError { - MagnusError::new(ruby.exception_range_error(), message.into()) +pub fn range_error(ruby: &Ruby, message: impl Into>) -> MagnusError { + MagnusError::new(ruby.exception_range_error(), message) } /// Build a `TypeError` from a conversion message. -pub fn type_error(ruby: &Ruby, message: impl Into) -> MagnusError { - MagnusError::new(ruby.exception_type_error(), message.into()) -} - -/// Select the most specific Ruby exception class for native predicate flags. -fn wreq_error_class(ruby: &Ruby, flags: &ErrorFlags) -> ExceptionClass { - let class = if flags.contains(ErrorFlags::IS_BUILDER) { - &BUILDER_ERROR - } else if flags.contains(ErrorFlags::IS_BODY) { - &BODY_ERROR - } else if flags.contains(ErrorFlags::IS_TLS) { - &TLS_ERROR - } else if flags.contains(ErrorFlags::IS_CONNECTION_RESET) { - &CONNECTION_RESET_ERROR - } else if flags.contains(ErrorFlags::IS_CONNECT) { - &CONNECTION_ERROR - } else if flags.contains(ErrorFlags::IS_PROXY_CONNECT) { - &PROXY_CONNECTION_ERROR - } else if flags.contains(ErrorFlags::IS_DECODE) { - &DECODING_ERROR - } else if flags.contains(ErrorFlags::IS_REDIRECT) { - &REDIRECT_ERROR - } else if flags.contains(ErrorFlags::IS_TIMEOUT) { - &TIMEOUT_ERROR - } else if flags.contains(ErrorFlags::IS_STATUS) { - &STATUS_ERROR - } else if flags.contains(ErrorFlags::IS_REQUEST) { - &REQUEST_ERROR - } else { - &WREQ_ERROR - }; - ruby.get_inner(class) +pub fn type_error(ruby: &Ruby, message: impl Into>) -> MagnusError { + MagnusError::new(ruby.exception_type_error(), message) +} + +/// Select the most specific Ruby exception class for native predicates. +fn wreq_error_class(ruby: &Ruby, predicates: ErrorPredicates) -> ExceptionClass { + for &predicate in ErrorPredicate::CLASSIFICATION_ORDER { + if predicates.contains(predicate) { + return ruby.get_inner(predicate.error_class()); + } + } + + ruby.get_inner(&WREQ_ERROR) } -/// Read one native predicate from a Ruby error, defaulting to `false`. -fn error_has_flag(rb_self: RObject, flag: ErrorFlags) -> Result { +/// Read one native predicate from a Ruby error, defaulting to false. +fn error_has_predicate(rb_self: RObject, predicate: ErrorPredicate) -> Result { rb_self - .ivar_get::<_, Option>(ERROR_FLAGS_IVAR) - .map(|flags| flags.is_some_and(|flags| ErrorFlags::from_bits_retain(flags).contains(flag))) + .ivar_get::<_, Option>(ERROR_PREDICATES_IVAR) + .map(|bits| bits.is_some_and(|bits| ErrorPredicates::from_bits(bits).contains(predicate))) } /// Construct a Ruby exception and attach immutable native error metadata. @@ -319,7 +349,7 @@ fn error_with_metadata( ) -> MagnusError { match class.new_instance((message,)).and_then(|exception| { let object = RObject::try_convert(exception.as_value())?; - object.ivar_set(ERROR_FLAGS_IVAR, metadata.flags.bits())?; + object.ivar_set(ERROR_PREDICATES_IVAR, metadata.predicates.bits())?; if let Some(uri) = metadata.uri { let uri = ruby.str_new(uri); @@ -340,8 +370,8 @@ fn error_with_metadata( /// Map [`wreq::Error`] to corresponding [`magnus::Error`]. pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { - let flags = wreq_error_flags(&err); - let class = wreq_error_class(ruby, &flags); + let predicates = ErrorPredicates::from(&err); + let class = wreq_error_class(ruby, predicates); let uri = err.uri().map(ToString::to_string); let status = err.status(); let message = err.without_uri().to_string(); @@ -353,7 +383,7 @@ pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { ErrorMetadata { uri: uri.as_deref(), status, - flags, + predicates, }, ) } @@ -375,47 +405,32 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), MagnusError> { initialize_exception!(ruby, gem_module, MEMORY, "MemoryError", error_class); initialize_exception!(ruby, gem_module, FORK_ERROR, "ForkError", error_class); - initialize_exception!( - ruby, - gem_module, - CONNECTION_ERROR, - "ConnectionError", - error_class - ); - initialize_exception!( - ruby, - gem_module, - PROXY_CONNECTION_ERROR, - "ProxyConnectionError", - error_class - ); - initialize_exception!( - ruby, - gem_module, - CONNECTION_RESET_ERROR, - "ConnectionResetError", - error_class - ); - initialize_exception!(ruby, gem_module, TLS_ERROR, "TlsError", error_class); - initialize_exception!(ruby, gem_module, REQUEST_ERROR, "RequestError", error_class); - - initialize_exception!(ruby, gem_module, STATUS_ERROR, "StatusError", error_class); - initialize_exception!( - ruby, - gem_module, - REDIRECT_ERROR, - "RedirectError", - error_class - ); - initialize_exception!(ruby, gem_module, TIMEOUT_ERROR, "TimeoutError", error_class); - initialize_exception!(ruby, gem_module, BODY_ERROR, "BodyError", error_class); - initialize_exception!( - ruby, - gem_module, - DECODING_ERROR, - "DecodingError", - error_class - ); - initialize_exception!(ruby, gem_module, BUILDER_ERROR, "BuilderError", error_class); + initialize_mapped_errors(ruby, gem_module, error_class)?; Ok(()) } + +#[cfg(test)] +mod tests { + use super::{ErrorPredicate, ErrorPredicates}; + + #[test] + fn error_predicate_bits_are_unique_and_round_trip() { + let predicates = ErrorPredicate::CLASSIFICATION_ORDER.iter().copied().fold( + ErrorPredicates::default(), + |predicates, predicate| { + assert!(!predicates.contains(predicate)); + predicates.include_if(predicate, true) + }, + ); + + assert_eq!( + ErrorPredicate::CLASSIFICATION_ORDER.len(), + predicates.bits().count_ones() as usize + ); + + let restored = ErrorPredicates::from_bits(predicates.bits()); + for &predicate in ErrorPredicate::CLASSIFICATION_ORDER { + assert!(restored.contains(predicate)); + } + } +} From 12729507f6b649ed1aa8fc20c5e0ec5267618206 Mon Sep 17 00:00:00 2001 From: gngpp Date: Fri, 24 Jul 2026 16:23:31 +0800 Subject: [PATCH 03/10] refactor(error): refine hierarchy API --- examples/error.rb | 6 +- lib/wreq_ruby/error.rb | 209 +++++++++-------------------------- lib/wreq_ruby/response.rb | 28 +++-- src/client/resp.rs | 21 ++-- src/error.rs | 2 +- test/error_hierarchy_test.rb | 13 ++- 6 files changed, 92 insertions(+), 187 deletions(-) diff --git a/examples/error.rb b/examples/error.rb index 8a39b69..597584a 100644 --- a/examples/error.rb +++ b/examples/error.rb @@ -5,8 +5,8 @@ begin Wreq.get("not-a-valid-url") rescue Wreq::Error => error - puts "URI: #{error.uri.inspect}" - puts "Status: #{error.status.inspect}" puts "#{error.class}: #{error.message}" - puts "Native builder error: #{error.is_builder}" + puts "builder: #{error.is_builder}" + puts "uri: #{error.uri.inspect}" + puts "status: #{error.status.inspect}" end diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 438c8a5..18834dc 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -2,244 +2,139 @@ unless defined?(Wreq) module Wreq - # Base class for regular errors raised by wreq-ruby. + # Base class for wreq-ruby runtime errors. # - # This remains a RuntimeError so existing broad rescue handlers continue - # to work. Predicate methods describe a captured native wreq::Error rather - # than the Ruby exception class, and more than one predicate may be true. - # Errors created entirely by the binding return false for every predicate. + # Error remains a RuntimeError so existing rescue handlers keep working. + # The `is_*` methods mirror predicates on the captured native `wreq::Error`. + # One error can match more than one predicate. Errors created by the binding + # itself return false for all of them. + # + # @example Rescue any wreq-ruby runtime error + # begin + # Wreq.get("not-a-valid-url") + # rescue Wreq::Error => error + # warn "#{error.class}: #{error.message}" + # end class Error < RuntimeError - # The URI associated with the native error. + # Get the URI recorded by the native error. # - # This explicit accessor may contain credentials or sensitive query - # parameters. Error messages and inspection output omit it, so avoid - # logging this value without redacting it first. + # This value may contain credentials, query parameters, or fragments. + # Error messages and `inspect` omit the URI. Redact it before logging it + # explicitly. # - # @return [String, nil] frozen URI string, if one was recorded + # @return [String, nil] Frozen URI string, if one was recorded attr_reader :uri - # The response status associated with the error. + # Get the HTTP status recorded by the native error. # # @return [Integer, nil] HTTP status code, if one was recorded attr_reader :status - # @return [Boolean] whether the native error came from a builder + # @return [Boolean] Whether the native error came from a builder def is_builder end - # @return [Boolean] whether the native error came from redirect handling + # @return [Boolean] Whether the native error came from redirect handling def is_redirect end - # @return [Boolean] whether the native error represents an HTTP status + # @return [Boolean] Whether the native error represents an HTTP status def is_status end - # @return [Boolean] whether the native error is related to a timeout + # @return [Boolean] Whether the native error is related to a timeout def is_timeout end - # @return [Boolean] whether the native error is related to a request + # @return [Boolean] Whether the native error is related to a request def is_request end - # @return [Boolean] whether the native error is related to connecting + # @return [Boolean] Whether the native error is related to connecting def is_connect end - # @return [Boolean] whether the native error is related to proxy connection + # @return [Boolean] Whether the native error is related to a proxy connection def is_proxy_connect end - # @return [Boolean] whether the native error is a connection reset + # @return [Boolean] Whether the native error is a connection reset def is_connection_reset end - # @return [Boolean] whether the native error is related to a body + # @return [Boolean] Whether the native error is related to a body def is_body end - # @return [Boolean] whether the native error is related to TLS + # @return [Boolean] Whether the native error is related to TLS def is_tls end - # @return [Boolean] whether the native error is related to decoding + # @return [Boolean] Whether the native error is related to decoding def is_decode end - # @return [Boolean] whether the native error is related to an upgrade + # @return [Boolean] Whether the native error is related to an upgrade def is_upgrade end end - # Raised when a native request wait is interrupted. + # Raised when Ruby interrupts a native request wait. # - # InterruptError stays outside StandardError so broad application rescues - # do not swallow Ruby interrupts. + # This inherits from Interrupt instead of Error, so `rescue StandardError` + # does not swallow the interrupt. class InterruptError < Interrupt; end - # System-level and runtime errors - - # Memory allocation failed. + # Raised when single-use native state was already consumed or is borrowed. class MemoryError < Error; end - # The child process inherited wreq-ruby from its parent. + # Raised when a forked child tries to use inherited native state. # - # Tokio worker threads do not survive fork, and inherited pooled - # connections are not safe to reuse. This error is raised before a child - # can access that state. - # - # @example - # Process.fork do - # Wreq::Client.new # Raises if the parent loaded wreq-ruby. - # end + # Tokio worker threads and pooled connections cannot be reused after fork. # @see https://github.com/SearchApi/wreq-ruby/blob/main/docs/fork-safety.md class ForkError < Error; end - # Network connection errors - - # Connection to the server failed. - # - # Raised when the client cannot establish a connection to the server. - # - # @example - # begin - # client.get("http://localhost:9999") - # rescue Wreq::ConnectionError => e - # puts "Connection failed: #{e.message}" - # retry_with_backoff - # end + # Raised when the client cannot connect to the destination server. class ConnectionError < Error; end - # Proxy Connection to the server failed. - # - # Raised when the client cannot establish a connection to the proxy server. - # @example - # begin - # client.get("http://example.com", proxy: "http://invalid-proxy:8080") - # rescue Wreq::ProxyConnectionError => e - # puts "Proxy connection failed: #{e.message}" - # retry_with_different_proxy - # end + # Raised when the client cannot connect to the configured proxy. class ProxyConnectionError < Error; end - # Connection was reset by the server. - # - # Raised when the server closes the connection unexpectedly. - # - # @example - # rescue Wreq::ConnectionResetError => e - # puts "Connection reset: #{e.message}" - # end + # Raised when a peer resets the connection. class ConnectionResetError < Error; end - # TLS/SSL error occurred. - # - # Raised when there's an error with TLS/SSL, such as certificate - # verification failure or protocol mismatch. - # - # @example - # begin - # client.get("https://self-signed.badssl.com") - # rescue Wreq::TlsError => e - # puts "TLS error: #{e.message}" - # end + # Raised when TLS negotiation or certificate verification fails. class TlsError < Error; end - # HTTP protocol and request/response errors - - # Request failed. - # - # Generic error for request failures that don't fit other categories. - # - # @example - # rescue Wreq::RequestError => e - # puts "Request failed: #{e.message}" - # end + # Raised for a request failure without a more specific error subclass. class RequestError < Error; end - # HTTP status code indicates an error. + # Raised when Response#raise_for_status! sees a 4xx or 5xx response. # - # Raised by Response#raise_for_status! for a 4xx or 5xx response. Requests - # continue to return these responses normally until that method is called. + # Requests return error responses normally until this opt-in check is made. + # The inherited `status` reader returns the integer HTTP status. # # @example # begin - # response = client.get("https://httpbin.io/status/404") - # response.raise_for_status! - # rescue Wreq::StatusError => e - # puts "HTTP #{e.status}: #{e.message}" + # client.get("https://example.com/missing").raise_for_status! + # rescue Wreq::StatusError => error + # warn "HTTP #{error.status}: #{error.message}" # end class StatusError < Error; end - # Redirect handling failed. - # - # Raised when too many redirects occur or redirect logic fails. - # - # @example - # begin - # client = Wreq::Client.new(allow_redirects: true, max_redirects: 3) - # client.get("https://httpbin.io/redirect/10") - # rescue Wreq::RedirectError => e - # puts "Too many redirects: #{e.message}" - # end + # Raised when redirect handling fails, such as after too many redirects. class RedirectError < Error; end - # Request timed out. - # - # Raised when the request exceeds the configured timeout. - # - # @example - # begin - # client = Wreq::Client.new(timeout: 5) - # client.get("https://httpbin.io/delay/10") - # rescue Wreq::TimeoutError => e - # puts "Request timed out: #{e.message}" - # retry_with_longer_timeout - # end + # Raised when a request operation exceeds its timeout. class TimeoutError < Error; end - # Data processing and encoding errors - - # Response body processing failed. - # - # Raised when there's an error reading or processing the response body. - # - # @example - # rescue Wreq::BodyError => e - # puts "Body error: #{e.message}" - # end + # Raised while sending, reading, or streaming an HTTP body. class BodyError < Error; end - # Decoding response failed. - # - # Raised when response content cannot be decoded (e.g., invalid UTF-8, - # malformed JSON, corrupted compression). - # - # @example - # begin - # response = client.get("https://example.com/invalid-utf8") - # response.text # May raise DecodingError - # rescue Wreq::DecodingError => e - # puts "Decoding error: #{e.message}" - # # Fall back to binary data - # data = response.body - # end + # Raised when a response body cannot be decoded or parsed. class DecodingError < Error; end - # Configuration and builder errors - - # A native client or request configuration could not be built. - # - # Raised when validated Ruby options cannot be represented by the native - # builder or request body. - # - # @example - # begin - # client = Wreq::Client.new(proxy: "invalid://") - # rescue Wreq::BuilderError => e - # puts "Invalid configuration: #{e.message}" - # end + # Raised when client, request, header, or body configuration is invalid. class BuilderError < Error; end end end diff --git a/lib/wreq_ruby/response.rb b/lib/wreq_ruby/response.rb index 24ba996..40a6421 100644 --- a/lib/wreq_ruby/response.rb +++ b/lib/wreq_ruby/response.rb @@ -2,15 +2,12 @@ unless defined?(Wreq) module Wreq - # HTTP response object containing status, headers, and body. + # An HTTP response returned by wreq-ruby. # - # This class wraps a native Rust implementation providing efficient - # access to HTTP response data including status codes, headers, body - # content, and streaming capabilities. - # - # Methods that use native response state, including body methods and - # {#raise_for_status!}, raise Wreq::ForkError if the child inherited - # wreq-ruby from its parent. + # Response metadata can be read repeatedly. Body helpers either buffer the + # body for reuse or stream it once. Body methods and {#raise_for_status!} + # raise Wreq::ForkError in a child that inherited the extension from its + # parent. # # @example Basic response handling # response = client.get("https://api.example.com") @@ -23,8 +20,8 @@ module Wreq # # @example Streaming response # response = client.get("https://example.com/large-file") - # response.stream.each do |chunk| - # # Process chunk + # File.open("download.bin", "wb") do |file| + # response.chunks { |chunk| file.write(chunk) } # end class Response # Get the HTTP status code as an integer. @@ -44,13 +41,14 @@ def code def status end - # Raise for a client or server error status. + # Return this response or raise for a 4xx or 5xx status. # - # This check is opt-in and does not consume the response body. + # Requests do not raise for HTTP status codes by default. This opt-in + # check leaves the response body available. # - # @return [Wreq::Response] This response for non-error statuses - # @raise [Wreq::StatusError] for a 4xx or 5xx status - # @raise [Wreq::ForkError] if the child inherited wreq-ruby from its parent + # @return [Wreq::Response] The same response for a non-error status + # @raise [Wreq::StatusError] If the status is in the 4xx or 5xx range + # @raise [Wreq::ForkError] If the child inherited wreq-ruby from its parent # @example # response = client.get("https://example.com/missing") # response.raise_for_status! diff --git a/src/client/resp.rs b/src/client/resp.rs index 6d6480d..17a00e8 100644 --- a/src/client/resp.rs +++ b/src/client/resp.rs @@ -71,7 +71,10 @@ impl Response { } } - /// Build a body-free native response for its status classification logic. + /// Build a body-free native response so wreq can construct a status error. + /// + /// This is used only for a client or server error status. Successful and + /// redirect responses avoid cloning the native extensions. fn response_for_status(&self) -> wreq::Response { let mut response = HttpResponse::new(Bytes::new()); *response.status_mut() = self.status.0; @@ -144,14 +147,18 @@ impl Response { /// Return this response unless its status is a client or server error. /// - /// Delegates classification to `wreq::Response::error_for_status_ref` - /// without consuming the response body. + /// Error construction is delegated to + /// [`wreq::Response::error_for_status_ref`] without consuming the body. pub fn raise_for_status(ruby: &Ruby, rb_self: Obj) -> Result, Error> { rt::ensure_current(ruby)?; - rb_self - .response_for_status() - .error_for_status_ref() - .map_err(|err| wreq_error(ruby, err))?; + + if rb_self.status.0.is_client_error() || rb_self.status.0.is_server_error() { + rb_self + .response_for_status() + .error_for_status_ref() + .map_err(|err| wreq_error(ruby, err))?; + } + Ok(rb_self) } diff --git a/src/error.rs b/src/error.rs index ce0b50f..1dc80b9 100644 --- a/src/error.rs +++ b/src/error.rs @@ -340,7 +340,7 @@ fn error_has_predicate(rb_self: RObject, predicate: ErrorPredicate) -> Result "ok", 302 => "redirect"}.each do |status, body| + with_status_server(status, body:) do |url| + response = Wreq.get(url) - assert_same response, response.raise_for_status! + assert_same response, response.raise_for_status! + assert_equal body, response.text + end end end @@ -236,7 +239,9 @@ def with_hanging_server def with_status_server(status, body: "") reason = { + 200 => "OK", 204 => "No Content", + 302 => "Found", 404 => "Not Found", 503 => "Service Unavailable" }.fetch(status) From 0a5d5fc7416c3f941f1776027a12c717fa26e96d Mon Sep 17 00:00:00 2001 From: gngpp Date: Fri, 24 Jul 2026 17:03:44 +0800 Subject: [PATCH 04/10] fix(error): correct status handling and examples --- lib/wreq_ruby/error.rb | 126 ++++++++++++++++++++++++++++++++++++++++- src/client/resp.rs | 21 +++---- 2 files changed, 131 insertions(+), 16 deletions(-) diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 18834dc..7663f13 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -14,6 +14,7 @@ module Wreq # Wreq.get("not-a-valid-url") # rescue Wreq::Error => error # warn "#{error.class}: #{error.message}" + # warn "invalid request" if error.is_builder # end class Error < RuntimeError # Get the URI recorded by the native error. @@ -83,30 +84,109 @@ def is_upgrade # # This inherits from Interrupt instead of Error, so `rescue StandardError` # does not swallow the interrupt. + # + # @example Handle an interrupted request separately + # begin + # Wreq.get("https://example.com", timeout: 30) + # rescue Wreq::InterruptError + # warn "request interrupted" + # rescue Wreq::Error => error + # warn error.message + # end class InterruptError < Interrupt; end # Raised when single-use native state was already consumed or is borrowed. + # + # @example A closed response no longer has a readable body + # response = Wreq.get("https://example.com") + # response.close + # response.bytes # Raises Wreq::MemoryError class MemoryError < Error; end # Raised when a forked child tries to use inherited native state. # # Tokio worker threads and pooled connections cannot be reused after fork. + # + # @example Native operations are rejected in an inherited child + # pid = Process.fork do + # begin + # Wreq::Client.new + # rescue Wreq::ForkError => error + # warn error.message + # end + # end + # Process.wait(pid) + # # @see https://github.com/SearchApi/wreq-ruby/blob/main/docs/fork-safety.md class ForkError < Error; end # Raised when the client cannot connect to the destination server. + # + # The error reflects the layer that actually fails. If a system proxy or + # VPN accepts the connection but does not return a response, the request + # raises Wreq::TimeoutError instead. + # + # @example Handle a destination connection failure + # client = Wreq::Client.new(no_proxy: true) + # begin + # client.get("http://127.0.0.1:1") + # rescue Wreq::ConnectionError => error + # warn "connection failed: #{error.message}" + # end class ConnectionError < Error; end # Raised when the client cannot connect to the configured proxy. + # + # @example Handle a proxy connection failure + # begin + # Wreq.get( + # "https://example.com", + # proxy: "http://127.0.0.1:1" + # ) + # rescue Wreq::ProxyConnectionError => error + # warn "proxy connection failed: #{error.message}" + # end class ProxyConnectionError < Error; end # Raised when a peer resets the connection. + # + # @example Handle a reset while streaming a response + # response = Wreq.get("https://example.com") + # begin + # File.open("response.bin", "wb") do |file| + # response.chunks { |chunk| file.write(chunk) } + # end + # rescue Wreq::ConnectionResetError => error + # warn "connection reset: #{error.message}" + # end class ConnectionResetError < Error; end - # Raised when TLS negotiation or certificate verification fails. + # Raised when native TLS setup fails while constructing a client. + # + # The current Ruby API does not expose certificate or identity inputs that + # can deliberately trigger this error. TLS handshake and certificate + # verification failures happen while connecting and normally raise + # Wreq::ConnectionError instead. + # + # @example Distinguish TLS setup errors from connection errors + # begin + # Wreq::Client.new(verify: true).get("https://example.com") + # rescue Wreq::TlsError => error + # warn "TLS setup failed: #{error.message}" + # rescue Wreq::ConnectionError => error + # warn "TLS connection failed: #{error.message}" + # end class TlsError < Error; end # Raised for a request failure without a more specific error subclass. + # + # @example Rescue the native fallback request category + # client = Wreq::Client.new + # begin + # client.get("https://example.com") + # rescue Wreq::RequestError => error + # warn "request failed: #{error.message}" + # end class RequestError < Error; end # Raised when Response#raise_for_status! sees a 4xx or 5xx response. @@ -115,26 +195,68 @@ class RequestError < Error; end # The inherited `status` reader returns the integer HTTP status. # # @example + # client = Wreq::Client.new # begin - # client.get("https://example.com/missing").raise_for_status! + # client.get("https://httpbin.io/status/404").raise_for_status! # rescue Wreq::StatusError => error # warn "HTTP #{error.status}: #{error.message}" # end class StatusError < Error; end # Raised when redirect handling fails, such as after too many redirects. + # + # @example Limit the number of redirects + # client = Wreq::Client.new(allow_redirects: true, max_redirects: 3) + # begin + # client.get("https://httpbin.io/redirect/10") + # rescue Wreq::RedirectError => error + # warn "redirect failed: #{error.message}" + # end class RedirectError < Error; end # Raised when a request operation exceeds its timeout. + # + # @example Handle a request timeout + # client = Wreq::Client.new(timeout: 1) + # begin + # client.get("https://httpbin.io/delay/10") + # rescue Wreq::TimeoutError => error + # warn "request timed out: #{error.message}" + # end class TimeoutError < Error; end # Raised while sending, reading, or streaming an HTTP body. + # + # @example Handle a body error while streaming + # response = Wreq.get("https://example.com") + # begin + # File.open("response.bin", "wb") do |file| + # response.chunks { |chunk| file.write(chunk) } + # end + # rescue Wreq::BodyError => error + # warn "body failed: #{error.message}" + # end class BodyError < Error; end # Raised when a response body cannot be decoded or parsed. + # + # @example Fall back to bytes when a response is not valid JSON + # response = Wreq.get("https://example.com") + # begin + # data = response.json + # rescue Wreq::DecodingError + # data = response.bytes + # end class DecodingError < Error; end # Raised when client, request, header, or body configuration is invalid. + # + # @example Handle an invalid request URL + # begin + # Wreq.get("not-a-valid-url") + # rescue Wreq::BuilderError => error + # warn "invalid request: #{error.message}" + # end class BuilderError < Error; end end end diff --git a/src/client/resp.rs b/src/client/resp.rs index 17a00e8..d1031f4 100644 --- a/src/client/resp.rs +++ b/src/client/resp.rs @@ -71,10 +71,7 @@ impl Response { } } - /// Build a body-free native response so wreq can construct a status error. - /// - /// This is used only for a client or server error status. Successful and - /// redirect responses avoid cloning the native extensions. + /// Build a body-free native response for its status classification logic. fn response_for_status(&self) -> wreq::Response { let mut response = HttpResponse::new(Bytes::new()); *response.status_mut() = self.status.0; @@ -147,18 +144,14 @@ impl Response { /// Return this response unless its status is a client or server error. /// - /// Error construction is delegated to - /// [`wreq::Response::error_for_status_ref`] without consuming the body. + /// Delegates classification to [`wreq::Response::error_for_status_ref`] + /// without consuming the response body. pub fn raise_for_status(ruby: &Ruby, rb_self: Obj) -> Result, Error> { rt::ensure_current(ruby)?; - - if rb_self.status.0.is_client_error() || rb_self.status.0.is_server_error() { - rb_self - .response_for_status() - .error_for_status_ref() - .map_err(|err| wreq_error(ruby, err))?; - } - + rb_self + .response_for_status() + .error_for_status_ref() + .map_err(|err| wreq_error(ruby, err))?; Ok(rb_self) } From 9f8d3b20f48e85bee6157ec38ea17e139483c29d Mon Sep 17 00:00:00 2001 From: gngpp Date: Fri, 31 Jul 2026 10:39:32 +0800 Subject: [PATCH 05/10] fix(error): classify overlapping native errors --- lib/wreq_ruby/error.rb | 40 ++++++-- src/error.rs | 175 ++++++++++++++++++++++++++++------- test/error_hierarchy_test.rb | 5 +- 3 files changed, 182 insertions(+), 38 deletions(-) diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index c98cd3e..43ab215 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -5,9 +5,17 @@ module Wreq # Base class for wreq-ruby runtime errors. # # Error remains a RuntimeError so existing rescue handlers keep working. - # The `is_*` methods mirror predicates on the captured native `wreq::Error`. - # One error can match more than one predicate. Errors created by the binding - # itself return false for all of them. + # Its subclass records one error category. The `is_*` methods preserve every + # predicate reported by the native `wreq::Error`, so more than one can be + # true. For example, a request timeout raises TimeoutError while both + # `is_timeout` and `is_request` are true. + # + # A native kind such as BodyError, TlsError, or StatusError takes precedence + # over details found in its cause chain. Native request errors are then + # classified as connection reset, timeout, proxy connection, destination + # connection, or RequestError, in that order. Use the predicates when code + # needs every native classification. Errors created by the binding itself + # return false for all of them. # # @example Rescue any wreq-ruby runtime error # begin @@ -43,10 +51,16 @@ def is_redirect def is_status end + # A request timeout uses TimeoutError even when this is also a connection + # or proxy connection error. + # # @return [Boolean] Whether the native error is related to a timeout def is_timeout end + # This may be true on connection and timeout subclasses because those + # errors occur while sending a request. + # # @return [Boolean] Whether the native error is related to a request def is_request end @@ -124,9 +138,10 @@ class ForkError < Error; end # Raised when the client cannot connect to the destination server. # - # The error reflects the layer that actually fails. If a system proxy or - # VPN accepts the connection but does not return a response, the request - # raises Wreq::TimeoutError instead. + # If the native error reports both a destination connection failure and a + # timeout, Wreq::TimeoutError is raised and `is_connect` remains true. A + # system proxy or VPN that accepts the connection but never responds may + # instead appear as a general request timeout. # # @example Handle a destination connection failure # client = Wreq::Client.new(no_proxy: true) @@ -139,6 +154,9 @@ class ConnectionError < Error; end # Raised when the client cannot connect to the configured proxy. # + # If the native error reports both a proxy connection failure and a timeout, + # Wreq::TimeoutError is raised and `is_proxy_connect` remains true. + # # @example Handle a proxy connection failure # begin # Wreq.get( @@ -182,6 +200,11 @@ class TlsError < Error; end # Raised for a request failure without a more specific error subclass. # + # Connection reset and timeout causes use their corresponding subclasses + # first. A reset takes precedence if both predicates are present. Other + # proxy and destination connection failures use their connection subclasses + # before this fallback. + # # @example Rescue the native fallback request category # client = Wreq::Client.new # begin @@ -218,6 +241,11 @@ class RedirectError < Error; end # Raised when a request operation exceeds its timeout. # + # This includes destination and proxy connection timeouts when the native + # error reports them as timeouts. Check `is_connect` or `is_proxy_connect` + # to see whether the native error also identifies that phase. `is_request` + # can be true on the same error. + # # @example Handle a request timeout # client = Wreq::Client.new(timeout: 1) # begin diff --git a/src/error.rs b/src/error.rs index 33ee2f1..4d240e1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -45,22 +45,42 @@ macro_rules! initialize_exception { } macro_rules! define_error_mapping { - ($($predicate:ident: $method:ident => $class:ident $(($ruby_name:literal))?),+ $(,)?) => { - /// Native predicates ordered by Ruby exception class priority. - #[derive(Clone, Copy)] + ( + $( + $predicate:ident [$role:ident]: + $method:ident => $class:ident $(($ruby_name:literal))? + ),+ $(,)? + ) => { + /// How a native predicate participates in Ruby exception classification. + #[derive(Clone, Copy, PartialEq, Eq)] + enum ErrorPredicateRole { + NativeKind, + RequestDetail, + } + + /// Predicates captured from a native `wreq::Error`. + #[derive(Clone, Copy, Debug, PartialEq, Eq)] #[repr(u8)] enum ErrorPredicate { $($predicate),+ } impl ErrorPredicate { - const CLASSIFICATION_ORDER: &'static [Self] = &[$(Self::$predicate),+]; + /// Every predicate retained in Ruby exception metadata. + const ALL: &'static [Self] = &[$(Self::$predicate),+]; /// Return this predicate's position in the compact Ruby metadata. const fn mask(self) -> u16 { 1_u16 << (self as u8) } + /// Return whether this is a native kind or a request detail. + const fn role(self) -> ErrorPredicateRole { + match self { + $(Self::$predicate => ErrorPredicateRole::$role,)+ + } + } + /// Evaluate this predicate before the native error is consumed. fn matches_wreq(self, error: &wreq::Error) -> bool { match self { @@ -68,7 +88,7 @@ macro_rules! define_error_mapping { } } - /// Return the Ruby class selected when this predicate has priority. + /// Return the Ruby class represented by this predicate. fn error_class(self) -> &'static Lazy { match self { $(Self::$predicate => &$class,)+ @@ -76,8 +96,7 @@ macro_rules! define_error_mapping { } } - const _: () = - assert!(ErrorPredicate::CLASSIFICATION_ORDER.len() <= u16::BITS as usize); + const _: () = assert!(ErrorPredicate::ALL.len() <= u16::BITS as usize); $( $(define_exception!($class, $ruby_name, exception_runtime_error);)? @@ -113,20 +132,24 @@ macro_rules! define_error_mapping { }; } -// The first matching entry determines the Ruby exception class. +// wreq keeps its error kind private. Keep its mutually exclusive kind predicates +// separate from request details, which inspect the source chain and may overlap. +// Entries within each role are classified from top to bottom. define_error_mapping! { - Builder: is_builder => BUILDER_ERROR("BuilderError"), - Body: is_body => BODY_ERROR("BodyError"), - Tls: is_tls => TLS_ERROR("TlsError"), - ConnectionReset: is_connection_reset => CONNECTION_RESET_ERROR("ConnectionResetError"), - Connect: is_connect => CONNECTION_ERROR("ConnectionError"), - ProxyConnect: is_proxy_connect => PROXY_CONNECTION_ERROR("ProxyConnectionError"), - Decode: is_decode => DECODING_ERROR("DecodingError"), - Redirect: is_redirect => REDIRECT_ERROR("RedirectError"), - Timeout: is_timeout => TIMEOUT_ERROR("TimeoutError"), - Status: is_status => STATUS_ERROR("StatusError"), - Request: is_request => REQUEST_ERROR("RequestError"), - Upgrade: is_upgrade => WREQ_ERROR, + Builder [NativeKind]: is_builder => BUILDER_ERROR("BuilderError"), + Body [NativeKind]: is_body => BODY_ERROR("BodyError"), + Tls [NativeKind]: is_tls => TLS_ERROR("TlsError"), + Decode [NativeKind]: is_decode => DECODING_ERROR("DecodingError"), + Redirect [NativeKind]: is_redirect => REDIRECT_ERROR("RedirectError"), + Status [NativeKind]: is_status => STATUS_ERROR("StatusError"), + Upgrade [NativeKind]: is_upgrade => WREQ_ERROR, + Request [NativeKind]: is_request => REQUEST_ERROR("RequestError"), + ConnectionReset [RequestDetail]: + is_connection_reset => CONNECTION_RESET_ERROR("ConnectionResetError"), + Timeout [RequestDetail]: is_timeout => TIMEOUT_ERROR("TimeoutError"), + ProxyConnect [RequestDetail]: + is_proxy_connect => PROXY_CONNECTION_ERROR("ProxyConnectionError"), + Connect [RequestDetail]: is_connect => CONNECTION_ERROR("ConnectionError"), } /// Native predicates retained after consuming a wreq error. @@ -157,12 +180,35 @@ impl ErrorPredicates { } self } + + /// Select one Ruby exception class without treating all predicates as peers. + /// + /// Native kinds are mutually exclusive. Connection and timeout predicates + /// only refine the request kind because they inspect the error source chain. + fn classifying_predicate(self) -> Option { + let kind = ErrorPredicate::ALL.iter().copied().find(|predicate| { + predicate.role() == ErrorPredicateRole::NativeKind && self.contains(*predicate) + })?; + + if kind == ErrorPredicate::Request { + ErrorPredicate::ALL + .iter() + .copied() + .find(|predicate| { + predicate.role() == ErrorPredicateRole::RequestDetail + && self.contains(*predicate) + }) + .or(Some(kind)) + } else { + Some(kind) + } + } } impl From<&wreq::Error> for ErrorPredicates { /// Snapshot every native predicate before consuming the wreq error. fn from(error: &wreq::Error) -> Self { - ErrorPredicate::CLASSIFICATION_ORDER + ErrorPredicate::ALL .iter() .copied() .fold(Self::default(), |predicates, predicate| { @@ -327,13 +373,10 @@ pub fn type_error(ruby: &Ruby, message: impl Into>) -> MagnusE /// Select the most specific Ruby exception class for native predicates. fn wreq_error_class(ruby: &Ruby, predicates: ErrorPredicates) -> ExceptionClass { - for &predicate in ErrorPredicate::CLASSIFICATION_ORDER { - if predicates.contains(predicate) { - return ruby.get_inner(predicate.error_class()); - } - } - - ruby.get_inner(&WREQ_ERROR) + predicates.classifying_predicate().map_or_else( + || ruby.get_inner(&WREQ_ERROR), + |predicate| ruby.get_inner(predicate.error_class()), + ) } /// Read one native predicate from a Ruby error, defaulting to false. @@ -416,9 +459,18 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), MagnusError> { mod tests { use super::{ErrorPredicate, ErrorPredicates}; + fn predicates(entries: &[ErrorPredicate]) -> ErrorPredicates { + entries + .iter() + .copied() + .fold(ErrorPredicates::default(), |predicates, predicate| { + predicates.include_if(predicate, true) + }) + } + #[test] fn error_predicate_bits_are_unique_and_round_trip() { - let predicates = ErrorPredicate::CLASSIFICATION_ORDER.iter().copied().fold( + let predicates = ErrorPredicate::ALL.iter().copied().fold( ErrorPredicates::default(), |predicates, predicate| { assert!(!predicates.contains(predicate)); @@ -427,13 +479,74 @@ mod tests { ); assert_eq!( - ErrorPredicate::CLASSIFICATION_ORDER.len(), + ErrorPredicate::ALL.len(), predicates.bits().count_ones() as usize ); let restored = ErrorPredicates::from_bits(predicates.bits()); - for &predicate in ErrorPredicate::CLASSIFICATION_ORDER { + for &predicate in ErrorPredicate::ALL { assert!(restored.contains(predicate)); } } + + #[test] + fn error_classification_separates_native_kinds_from_request_details() { + let cases: &[(&[ErrorPredicate], Option)] = &[ + (&[], None), + (&[ErrorPredicate::Upgrade], Some(ErrorPredicate::Upgrade)), + (&[ErrorPredicate::Request], Some(ErrorPredicate::Request)), + ( + &[ErrorPredicate::Request, ErrorPredicate::Connect], + Some(ErrorPredicate::Connect), + ), + ( + &[ErrorPredicate::Request, ErrorPredicate::ProxyConnect], + Some(ErrorPredicate::ProxyConnect), + ), + ( + &[ErrorPredicate::Request, ErrorPredicate::Timeout], + Some(ErrorPredicate::Timeout), + ), + ( + &[ + ErrorPredicate::Request, + ErrorPredicate::Connect, + ErrorPredicate::Timeout, + ], + Some(ErrorPredicate::Timeout), + ), + ( + &[ + ErrorPredicate::Request, + ErrorPredicate::ProxyConnect, + ErrorPredicate::Timeout, + ], + Some(ErrorPredicate::Timeout), + ), + ( + &[ + ErrorPredicate::Request, + ErrorPredicate::ConnectionReset, + ErrorPredicate::Timeout, + ], + Some(ErrorPredicate::ConnectionReset), + ), + ( + &[ + ErrorPredicate::Body, + ErrorPredicate::Request, + ErrorPredicate::Timeout, + ], + Some(ErrorPredicate::Body), + ), + ]; + + for &(entries, expected) in cases { + assert_eq!( + expected, + predicates(entries).classifying_predicate(), + "predicates: {entries:?}" + ); + } + } } diff --git a/test/error_hierarchy_test.rb b/test/error_hierarchy_test.rb index 0e4c1d9..a21fa7c 100644 --- a/test/error_hierarchy_test.rb +++ b/test/error_hierarchy_test.rb @@ -70,11 +70,14 @@ def test_binding_generated_errors_have_no_native_predicates end def test_native_error_predicates_are_not_mutually_exclusive + client = Wreq::Client.new(no_proxy: true) + with_hanging_server do |url, _accepted| - error = assert_raises(Wreq::TimeoutError) { Wreq.get(url, timeout: 1) } + error = assert_raises(Wreq::TimeoutError) { client.get(url, timeout: 1) } assert error.is_timeout assert error.is_request + assert_equal %i[is_timeout is_request], active_native_predicates(error) end end From e6bde9140bf81d60756c01a9e04a9bddfa080bf0 Mon Sep 17 00:00:00 2001 From: 0x676e67 Date: Fri, 31 Jul 2026 15:28:57 +0800 Subject: [PATCH 06/10] refactor(error): expand predicate metadata capacity (#153) --- src/error.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/error.rs b/src/error.rs index 33ee2f1..b98946e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -11,6 +11,7 @@ use magnus::{ use tokio::sync::mpsc::error::SendError; const ERROR_PREDICATES_IVAR: &str = "wreq_error_predicates"; +type ErrorPredicateBits = u64; const RACE_CONDITION_ERROR_MSG: &str = r#"Due to Rust's memory management with borrowing, you cannot use certain instances multiple times as they may be consumed. @@ -57,8 +58,8 @@ macro_rules! define_error_mapping { const CLASSIFICATION_ORDER: &'static [Self] = &[$(Self::$predicate),+]; /// Return this predicate's position in the compact Ruby metadata. - const fn mask(self) -> u16 { - 1_u16 << (self as u8) + const fn mask(self) -> ErrorPredicateBits { + 1 << (self as u8) } /// Evaluate this predicate before the native error is consumed. @@ -77,7 +78,7 @@ macro_rules! define_error_mapping { } const _: () = - assert!(ErrorPredicate::CLASSIFICATION_ORDER.len() <= u16::BITS as usize); + assert!(ErrorPredicate::CLASSIFICATION_ORDER.len() <= ErrorPredicateBits::BITS as usize); $( $(define_exception!($class, $ruby_name, exception_runtime_error);)? @@ -131,16 +132,16 @@ define_error_mapping! { /// Native predicates retained after consuming a wreq error. #[derive(Clone, Copy, Default)] -struct ErrorPredicates(u16); +struct ErrorPredicates(ErrorPredicateBits); impl ErrorPredicates { /// Restore predicates from compact Ruby metadata. - const fn from_bits(bits: u16) -> Self { + const fn from_bits(bits: ErrorPredicateBits) -> Self { Self(bits) } /// Return the compact representation stored on the Ruby exception. - const fn bits(self) -> u16 { + const fn bits(self) -> ErrorPredicateBits { self.0 } @@ -339,7 +340,7 @@ fn wreq_error_class(ruby: &Ruby, predicates: ErrorPredicates) -> ExceptionClass /// Read one native predicate from a Ruby error, defaulting to false. fn error_has_predicate(rb_self: RObject, predicate: ErrorPredicate) -> Result { rb_self - .ivar_get::<_, Option>(ERROR_PREDICATES_IVAR) + .ivar_get::<_, Option>(ERROR_PREDICATES_IVAR) .map(|bits| bits.is_some_and(|bits| ErrorPredicates::from_bits(bits).contains(predicate))) } From a6a16c7505585554061e690a6abd042ab6df34f5 Mon Sep 17 00:00:00 2001 From: 0x676e67 Date: Fri, 31 Jul 2026 16:04:53 +0800 Subject: [PATCH 07/10] refactor(error): use idiomatic Ruby predicates (#155) --- examples/error.rb | 2 +- lib/wreq_ruby/error.rb | 42 ++++++++++++++++++------------------ src/error.rs | 41 ++++++++++++++++++++--------------- test/error_hierarchy_test.rb | 40 +++++++++++++++++----------------- 4 files changed, 66 insertions(+), 59 deletions(-) diff --git a/examples/error.rb b/examples/error.rb index 597584a..8d53189 100644 --- a/examples/error.rb +++ b/examples/error.rb @@ -6,7 +6,7 @@ Wreq.get("not-a-valid-url") rescue Wreq::Error => error puts "#{error.class}: #{error.message}" - puts "builder: #{error.is_builder}" + puts "builder: #{error.builder?}" puts "uri: #{error.uri.inspect}" puts "status: #{error.status.inspect}" end diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 43ab215..4a7be17 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -5,10 +5,10 @@ module Wreq # Base class for wreq-ruby runtime errors. # # Error remains a RuntimeError so existing rescue handlers keep working. - # Its subclass records one error category. The `is_*` methods preserve every - # predicate reported by the native `wreq::Error`, so more than one can be - # true. For example, a request timeout raises TimeoutError while both - # `is_timeout` and `is_request` are true. + # Its subclass records one primary category. Predicate methods retain every + # classification reported by the native `wreq::Error`, so more than one can + # be true. For example, a request timeout raises TimeoutError while both + # `timeout?` and `request?` return true. # # A native kind such as BodyError, TlsError, or StatusError takes precedence # over details found in its cause chain. Native request errors are then @@ -22,7 +22,7 @@ module Wreq # Wreq.get("not-a-valid-url") # rescue Wreq::Error => error # warn "#{error.class}: #{error.message}" - # warn "invalid request" if error.is_builder + # warn "invalid request" if error.builder? # end class Error < RuntimeError # Get the URI recorded by the native error. @@ -40,57 +40,57 @@ class Error < RuntimeError attr_reader :status # @return [Boolean] Whether the native error came from a builder - def is_builder + def builder? end # @return [Boolean] Whether the native error came from redirect handling - def is_redirect + def redirect? end # @return [Boolean] Whether the native error represents an HTTP status - def is_status + def status? end # A request timeout uses TimeoutError even when this is also a connection # or proxy connection error. # # @return [Boolean] Whether the native error is related to a timeout - def is_timeout + def timeout? end # This may be true on connection and timeout subclasses because those # errors occur while sending a request. # # @return [Boolean] Whether the native error is related to a request - def is_request + def request? end # @return [Boolean] Whether the native error is related to connecting - def is_connect + def connection? end # @return [Boolean] Whether the native error is related to a proxy connection - def is_proxy_connect + def proxy_connection? end # @return [Boolean] Whether the native error is a connection reset - def is_connection_reset + def connection_reset? end # @return [Boolean] Whether the native error is related to a body - def is_body + def body? end # @return [Boolean] Whether the native error is related to TLS - def is_tls + def tls? end # @return [Boolean] Whether the native error is related to decoding - def is_decode + def decoding? end # @return [Boolean] Whether the native error is related to an upgrade - def is_upgrade + def upgrade? end end @@ -139,7 +139,7 @@ class ForkError < Error; end # Raised when the client cannot connect to the destination server. # # If the native error reports both a destination connection failure and a - # timeout, Wreq::TimeoutError is raised and `is_connect` remains true. A + # timeout, Wreq::TimeoutError is raised and `connection?` remains true. A # system proxy or VPN that accepts the connection but never responds may # instead appear as a general request timeout. # @@ -155,7 +155,7 @@ class ConnectionError < Error; end # Raised when the client cannot connect to the configured proxy. # # If the native error reports both a proxy connection failure and a timeout, - # Wreq::TimeoutError is raised and `is_proxy_connect` remains true. + # Wreq::TimeoutError is raised and `proxy_connection?` remains true. # # @example Handle a proxy connection failure # begin @@ -242,8 +242,8 @@ class RedirectError < Error; end # Raised when a request operation exceeds its timeout. # # This includes destination and proxy connection timeouts when the native - # error reports them as timeouts. Check `is_connect` or `is_proxy_connect` - # to see whether the native error also identifies that phase. `is_request` + # error reports them as timeouts. Check `connection?` or `proxy_connection?` + # to see whether the native error also identifies that phase. `request?` # can be true on the same error. # # @example Handle a request timeout diff --git a/src/error.rs b/src/error.rs index a66ed87..1f533d6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -49,7 +49,8 @@ macro_rules! define_error_mapping { ( $( $predicate:ident [$role:ident]: - $method:ident => $class:ident $(($ruby_name:literal))? + $native_method:ident as $ruby_method:ident + => $class:ident $(($ruby_name:literal))? ),+ $(,)? ) => { /// How a native predicate participates in Ruby exception classification. @@ -85,7 +86,7 @@ macro_rules! define_error_mapping { /// Evaluate this predicate before the native error is consumed. fn matches_wreq(self, error: &wreq::Error) -> bool { match self { - $(Self::$predicate => error.$method(),)+ + $(Self::$predicate => error.$native_method(),)+ } } @@ -105,15 +106,18 @@ macro_rules! define_error_mapping { )+ $( - fn $method(rb_self: RObject) -> Result { + fn $native_method(rb_self: RObject) -> Result { error_has_predicate(rb_self, ErrorPredicate::$predicate) } )+ - /// Define the native wreq predicate methods on Wreq::Error. + /// Define idiomatic Ruby predicate methods on `Wreq::Error`. fn include_error_predicates(class: ExceptionClass) -> Result<(), MagnusError> { $( - class.define_method(stringify!($method), magnus::method!($method, 0))?; + class.define_method( + concat!(stringify!($ruby_method), "?"), + magnus::method!($native_method, 0), + )?; )+ Ok(()) } @@ -136,22 +140,25 @@ macro_rules! define_error_mapping { // wreq keeps its error kind private. Keep its mutually exclusive kind predicates // separate from request details, which inspect the source chain and may overlap. +// Each entry maps the native method before `as` to the Ruby predicate after it. // Entries within each role are classified from top to bottom. define_error_mapping! { - Builder [NativeKind]: is_builder => BUILDER_ERROR("BuilderError"), - Body [NativeKind]: is_body => BODY_ERROR("BodyError"), - Tls [NativeKind]: is_tls => TLS_ERROR("TlsError"), - Decode [NativeKind]: is_decode => DECODING_ERROR("DecodingError"), - Redirect [NativeKind]: is_redirect => REDIRECT_ERROR("RedirectError"), - Status [NativeKind]: is_status => STATUS_ERROR("StatusError"), - Upgrade [NativeKind]: is_upgrade => WREQ_ERROR, - Request [NativeKind]: is_request => REQUEST_ERROR("RequestError"), + Builder [NativeKind]: is_builder as builder => BUILDER_ERROR("BuilderError"), + Body [NativeKind]: is_body as body => BODY_ERROR("BodyError"), + Tls [NativeKind]: is_tls as tls => TLS_ERROR("TlsError"), + Decode [NativeKind]: is_decode as decoding => DECODING_ERROR("DecodingError"), + Redirect [NativeKind]: is_redirect as redirect => REDIRECT_ERROR("RedirectError"), + Status [NativeKind]: is_status as status => STATUS_ERROR("StatusError"), + Upgrade [NativeKind]: is_upgrade as upgrade => WREQ_ERROR, + Request [NativeKind]: is_request as request => REQUEST_ERROR("RequestError"), ConnectionReset [RequestDetail]: - is_connection_reset => CONNECTION_RESET_ERROR("ConnectionResetError"), - Timeout [RequestDetail]: is_timeout => TIMEOUT_ERROR("TimeoutError"), + is_connection_reset as connection_reset + => CONNECTION_RESET_ERROR("ConnectionResetError"), + Timeout [RequestDetail]: is_timeout as timeout => TIMEOUT_ERROR("TimeoutError"), ProxyConnect [RequestDetail]: - is_proxy_connect => PROXY_CONNECTION_ERROR("ProxyConnectionError"), - Connect [RequestDetail]: is_connect => CONNECTION_ERROR("ConnectionError"), + is_proxy_connect as proxy_connection + => PROXY_CONNECTION_ERROR("ProxyConnectionError"), + Connect [RequestDetail]: is_connect as connection => CONNECTION_ERROR("ConnectionError"), } /// Native predicates retained after consuming a wreq error. diff --git a/test/error_hierarchy_test.rb b/test/error_hierarchy_test.rb index a21fa7c..9f5214c 100644 --- a/test/error_hierarchy_test.rb +++ b/test/error_hierarchy_test.rb @@ -19,18 +19,18 @@ class ErrorHierarchyTest < Minitest::Test BuilderError ].freeze NATIVE_ERROR_PREDICATES = %i[ - is_builder - is_redirect - is_status - is_timeout - is_request - is_connect - is_proxy_connect - is_connection_reset - is_body - is_tls - is_decode - is_upgrade + builder? + redirect? + status? + timeout? + request? + connection? + proxy_connection? + connection_reset? + body? + tls? + decoding? + upgrade? ].freeze def test_regular_errors_share_stable_root @@ -57,9 +57,9 @@ def test_root_and_specific_errors_can_be_rescued end assert_instance_of Wreq::BuilderError, root_error - assert root_error.is_builder + assert_predicate root_error, :builder? assert_nil root_error.status - assert_equal [:is_builder], active_native_predicates(root_error) + assert_equal [:builder?], active_native_predicates(root_error) assert_raises(Wreq::BuilderError) { Wreq.get("not-a-valid-url") } end @@ -75,9 +75,9 @@ def test_native_error_predicates_are_not_mutually_exclusive with_hanging_server do |url, _accepted| error = assert_raises(Wreq::TimeoutError) { client.get(url, timeout: 1) } - assert error.is_timeout - assert error.is_request - assert_equal %i[is_timeout is_request], active_native_predicates(error) + assert_predicate error, :timeout? + assert_predicate error, :request? + assert_equal %i[timeout? request?], active_native_predicates(error) end end @@ -137,8 +137,8 @@ def test_raise_for_status_exposes_status_without_consuming_body assert_equal status, error.status assert_equal response.url, error.uri assert_predicate error.uri, :frozen? - assert error.is_status - assert_equal [:is_status], active_native_predicates(error) + assert_predicate error, :status? + assert_equal [:status?], active_native_predicates(error) refute_respond_to error, :kind refute_respond_to error, :retryable? refute_includes error.message, "response-secret" @@ -203,7 +203,7 @@ def test_option_context_preserves_native_error_metadata error = assert_raises(Wreq::BuilderError) { Wreq::Client.new(proxy: "://") } assert_includes error.message, ":proxy" - assert_equal [:is_builder], active_native_predicates(error) + assert_equal [:builder?], active_native_predicates(error) end private From e04a520b2d452a9bee18b5dedcb9679c9c4e97fa Mon Sep 17 00:00:00 2001 From: gngpp Date: Mon, 3 Aug 2026 17:21:20 +0800 Subject: [PATCH 08/10] refactor(error)!: use connect-stage terminology BREAKING CHANGE: Wreq::ConnectionError and Wreq::ProxyConnectionError are replaced by Wreq::ConnectError and Wreq::ProxyConnectError. The connection? and proxy_connection? predicates are replaced by connect? and proxy_connect?. --- lib/wreq_ruby/error.rb | 48 +++++++++++++++++++----------------- src/error.rs | 6 ++--- test/error_handling_test.rb | 15 ++++++----- test/error_hierarchy_test.rb | 12 ++++++--- 4 files changed, 44 insertions(+), 37 deletions(-) diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 4a7be17..174213a 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -12,10 +12,10 @@ module Wreq # # A native kind such as BodyError, TlsError, or StatusError takes precedence # over details found in its cause chain. Native request errors are then - # classified as connection reset, timeout, proxy connection, destination - # connection, or RequestError, in that order. Use the predicates when code - # needs every native classification. Errors created by the binding itself - # return false for all of them. + # classified as connection reset, timeout, proxy connect failure, + # destination connect failure, or RequestError, in that order. Use the + # predicates when code needs every native classification. Errors created by + # the binding itself return false for all of them. # # @example Rescue any wreq-ruby runtime error # begin @@ -65,12 +65,14 @@ def timeout? def request? end - # @return [Boolean] Whether the native error is related to connecting - def connection? + # @return [Boolean] Whether the native error occurred while acquiring a + # connection to the destination + def connect? end - # @return [Boolean] Whether the native error is related to a proxy connection - def proxy_connection? + # @return [Boolean] Whether the native error occurred while connecting + # through a proxy + def proxy_connect? end # @return [Boolean] Whether the native error is a connection reset @@ -136,10 +138,11 @@ class MemoryError < Error; end # @see https://github.com/SearchApi/wreq-ruby/blob/main/docs/fork-safety.md class ForkError < Error; end - # Raised when the client cannot connect to the destination server. + # Raised when the client cannot acquire a usable connection to the + # destination server. # # If the native error reports both a destination connection failure and a - # timeout, Wreq::TimeoutError is raised and `connection?` remains true. A + # timeout, Wreq::TimeoutError is raised and `connect?` remains true. A # system proxy or VPN that accepts the connection but never responds may # instead appear as a general request timeout. # @@ -147,15 +150,17 @@ class ForkError < Error; end # client = Wreq::Client.new(no_proxy: true) # begin # client.get("http://127.0.0.1:1") - # rescue Wreq::ConnectionError => error + # rescue Wreq::ConnectError => error # warn "connection failed: #{error.message}" # end - class ConnectionError < Error; end + class ConnectError < Error; end - # Raised when the client cannot connect to the configured proxy. + # Raised when the client cannot establish a connection through the + # configured proxy. This includes failures while connecting to the proxy or + # negotiating a proxy tunnel. # # If the native error reports both a proxy connection failure and a timeout, - # Wreq::TimeoutError is raised and `proxy_connection?` remains true. + # Wreq::TimeoutError is raised and `proxy_connect?` remains true. # # @example Handle a proxy connection failure # begin @@ -163,10 +168,10 @@ class ConnectionError < Error; end # "https://example.com", # proxy: "http://127.0.0.1:1" # ) - # rescue Wreq::ProxyConnectionError => error + # rescue Wreq::ProxyConnectError => error # warn "proxy connection failed: #{error.message}" # end - class ProxyConnectionError < Error; end + class ProxyConnectError < Error; end # Raised when a peer resets the connection. # @@ -186,14 +191,14 @@ class ConnectionResetError < Error; end # The current Ruby API does not expose certificate or identity inputs that # can deliberately trigger this error. TLS handshake and certificate # verification failures happen while connecting and normally raise - # Wreq::ConnectionError instead. + # Wreq::ConnectError instead. # # @example Distinguish TLS setup errors from connection errors # begin # Wreq::Client.new(verify: true).get("https://example.com") # rescue Wreq::TlsError => error # warn "TLS setup failed: #{error.message}" - # rescue Wreq::ConnectionError => error + # rescue Wreq::ConnectError => error # warn "TLS connection failed: #{error.message}" # end class TlsError < Error; end @@ -241,10 +246,9 @@ class RedirectError < Error; end # Raised when a request operation exceeds its timeout. # - # This includes destination and proxy connection timeouts when the native - # error reports them as timeouts. Check `connection?` or `proxy_connection?` - # to see whether the native error also identifies that phase. `request?` - # can be true on the same error. + # This includes timeouts while connecting to the destination or proxy. Check + # `connect?` or `proxy_connect?` to see whether the native error also + # identifies the connect phase. `request?` can be true on the same error. # # @example Handle a request timeout # client = Wreq::Client.new(timeout: 1) diff --git a/src/error.rs b/src/error.rs index 1f533d6..243d395 100644 --- a/src/error.rs +++ b/src/error.rs @@ -156,9 +156,9 @@ define_error_mapping! { => CONNECTION_RESET_ERROR("ConnectionResetError"), Timeout [RequestDetail]: is_timeout as timeout => TIMEOUT_ERROR("TimeoutError"), ProxyConnect [RequestDetail]: - is_proxy_connect as proxy_connection - => PROXY_CONNECTION_ERROR("ProxyConnectionError"), - Connect [RequestDetail]: is_connect as connection => CONNECTION_ERROR("ConnectionError"), + is_proxy_connect as proxy_connect + => PROXY_CONNECT_ERROR("ProxyConnectError"), + Connect [RequestDetail]: is_connect as connect => CONNECT_ERROR("ConnectError"), } /// Native predicates retained after consuming a wreq error. diff --git a/test/error_handling_test.rb b/test/error_handling_test.rb index e229dba..2d37a2f 100644 --- a/test/error_handling_test.rb +++ b/test/error_handling_test.rb @@ -32,13 +32,12 @@ def test_request_interruption_raises_interrupt_error request_thread&.join(1) end - def test_network_error_handling - # Try to connect to a non-existent domain + def test_connect_error_handling + # wreq classifies DNS failures as connect errors. response = Wreq.get("https://definitely-not-a-real-domain-12345.com") flunk "Expected network error but got response: #{response.code}" rescue => e - assert_instance_of Wreq::ConnectionError, e - # Network errors should be caught and wrapped appropriately + assert_instance_of Wreq::ConnectError, e end def test_invalid_url_handling @@ -97,7 +96,7 @@ def test_empty_response_json end end - def test_proxy_error_handling + def test_proxy_connect_error_handling invalid_proxies = [ "http://invalid.proxy:8080", "https://invalid.proxy:8080", @@ -111,11 +110,11 @@ def test_proxy_error_handling invalid_proxies.each do |proxy| target_urls.each do |url| Wreq.get(url, proxy: proxy, timeout: 5) - flunk "Expected proxy connection error but got response" + flunk "Expected proxy connect error but got response" rescue => e assert( - e.is_a?(Wreq::ProxyConnectionError) || e.is_a?(Wreq::RequestError), - "Expected ProxyConnectionError or RequestError, got #{e.class}: #{e.message}" + e.is_a?(Wreq::ProxyConnectError) || e.is_a?(Wreq::RequestError), + "Expected ProxyConnectError or RequestError, got #{e.class}: #{e.message}" ) end end diff --git a/test/error_hierarchy_test.rb b/test/error_hierarchy_test.rb index 9f5214c..9901e0c 100644 --- a/test/error_hierarchy_test.rb +++ b/test/error_hierarchy_test.rb @@ -6,8 +6,8 @@ class ErrorHierarchyTest < Minitest::Test REGULAR_ERROR_NAMES = %i[ MemoryError ForkError - ConnectionError - ProxyConnectionError + ConnectError + ProxyConnectError ConnectionResetError TlsError RequestError @@ -24,8 +24,8 @@ class ErrorHierarchyTest < Minitest::Test status? timeout? request? - connection? - proxy_connection? + connect? + proxy_connect? connection_reset? body? tls? @@ -40,10 +40,14 @@ def test_regular_errors_share_stable_root REGULAR_ERROR_NAMES.each do |name| assert_equal Wreq::Error, Wreq.const_get(name).superclass end + refute Wreq.const_defined?(:ConnectionError, false) + refute Wreq.const_defined?(:ProxyConnectionError, false) error = Wreq::MemoryError.new assert_nil error.uri assert_nil error.status + refute_respond_to error, :connection? + refute_respond_to error, :proxy_connection? NATIVE_ERROR_PREDICATES.each do |predicate| assert_equal false, error.public_send(predicate) end From b4d4ceb3eced11b015193d8233aa83b02d430c9e Mon Sep 17 00:00:00 2001 From: gngpp Date: Mon, 3 Aug 2026 18:31:34 +0800 Subject: [PATCH 09/10] fix(error): define stable classification contract --- lib/wreq_ruby/error.rb | 9 ++-- src/error.rs | 101 ++++++++++++++++++++++++++++++----- test/error_hierarchy_test.rb | 50 +++++++++++++++-- 3 files changed, 140 insertions(+), 20 deletions(-) diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index 174213a..cdc0e09 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -13,9 +13,12 @@ module Wreq # A native kind such as BodyError, TlsError, or StatusError takes precedence # over details found in its cause chain. Native request errors are then # classified as connection reset, timeout, proxy connect failure, - # destination connect failure, or RequestError, in that order. Use the - # predicates when code needs every native classification. Errors created by - # the binding itself return false for all of them. + # destination connect failure, or RequestError, in that order. This order is + # defined by wreq-ruby and does not depend on the order of native checks. Use + # the predicates when code needs every native classification. Errors created + # by the binding itself return false for all of them. New native facts may be + # added as predicates without changing the exception class for existing + # failures. # # @example Rescue any wreq-ruby runtime error # begin diff --git a/src/error.rs b/src/error.rs index 243d395..bcec592 100644 --- a/src/error.rs +++ b/src/error.rs @@ -53,11 +53,13 @@ macro_rules! define_error_mapping { => $class:ident $(($ruby_name:literal))? ),+ $(,)? ) => { - /// How a native predicate participates in Ruby exception classification. - #[derive(Clone, Copy, PartialEq, Eq)] + /// How a native predicate participates in the Ruby error contract. + #[cfg(test)] + #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum ErrorPredicateRole { NativeKind, RequestDetail, + Diagnostic, } /// Predicates captured from a native `wreq::Error`. @@ -77,6 +79,7 @@ macro_rules! define_error_mapping { } /// Return whether this is a native kind or a request detail. + #[cfg(test)] const fn role(self) -> ErrorPredicateRole { match self { $(Self::$predicate => ErrorPredicateRole::$role,)+ @@ -141,7 +144,7 @@ macro_rules! define_error_mapping { // wreq keeps its error kind private. Keep its mutually exclusive kind predicates // separate from request details, which inspect the source chain and may overlap. // Each entry maps the native method before `as` to the Ruby predicate after it. -// Entries within each role are classified from top to bottom. +// Classification order is declared separately as part of the Ruby contract. define_error_mapping! { Builder [NativeKind]: is_builder as builder => BUILDER_ERROR("BuilderError"), Body [NativeKind]: is_body as body => BODY_ERROR("BodyError"), @@ -161,6 +164,36 @@ define_error_mapping! { Connect [RequestDetail]: is_connect as connect => CONNECT_ERROR("ConnectError"), } +/// Stable precedence for mutually exclusive native error kinds. +/// +/// This order belongs to the Ruby API. Reordering the mapping macro or changing +/// the order in which wreq evaluates predicates must not change rescue behavior. +const RUBY_ERROR_KIND_PRIORITY: &[ErrorPredicate] = &[ + ErrorPredicate::Builder, + ErrorPredicate::Body, + ErrorPredicate::Tls, + ErrorPredicate::Decode, + ErrorPredicate::Redirect, + ErrorPredicate::Status, + ErrorPredicate::Upgrade, + ErrorPredicate::Request, +]; + +/// Stable precedence for details that refine a native request error. +const RUBY_REQUEST_DETAIL_PRIORITY: &[ErrorPredicate] = &[ + ErrorPredicate::ConnectionReset, + ErrorPredicate::Timeout, + ErrorPredicate::ProxyConnect, + ErrorPredicate::Connect, +]; + +/// Native facts exposed to Ruby without changing the exception class. +/// +/// Add new overlapping predicates here unless a major release intentionally +/// changes which exception existing rescue clauses receive. +#[cfg(test)] +const RUBY_DIAGNOSTIC_PREDICATES: &[ErrorPredicate] = &[]; + /// Native predicates retained after consuming a wreq error. #[derive(Clone, Copy, Default)] struct ErrorPredicates(ErrorPredicateBits); @@ -190,23 +223,23 @@ impl ErrorPredicates { self } + /// Return the first captured predicate in a binding-owned priority list. + fn first_present(self, priority: &[ErrorPredicate]) -> Option { + priority + .iter() + .copied() + .find(|predicate| self.contains(*predicate)) + } + /// Select one Ruby exception class without treating all predicates as peers. /// /// Native kinds are mutually exclusive. Connection and timeout predicates /// only refine the request kind because they inspect the error source chain. fn classifying_predicate(self) -> Option { - let kind = ErrorPredicate::ALL.iter().copied().find(|predicate| { - predicate.role() == ErrorPredicateRole::NativeKind && self.contains(*predicate) - })?; + let kind = self.first_present(RUBY_ERROR_KIND_PRIORITY)?; if kind == ErrorPredicate::Request { - ErrorPredicate::ALL - .iter() - .copied() - .find(|predicate| { - predicate.role() == ErrorPredicateRole::RequestDetail - && self.contains(*predicate) - }) + self.first_present(RUBY_REQUEST_DETAIL_PRIORITY) .or(Some(kind)) } else { Some(kind) @@ -466,7 +499,10 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), MagnusError> { #[cfg(test)] mod tests { - use super::{ErrorPredicate, ErrorPredicates}; + use super::{ + ErrorPredicate, ErrorPredicateRole, ErrorPredicates, RUBY_DIAGNOSTIC_PREDICATES, + RUBY_ERROR_KIND_PRIORITY, RUBY_REQUEST_DETAIL_PRIORITY, + }; fn predicates(entries: &[ErrorPredicate]) -> ErrorPredicates { entries @@ -498,6 +534,35 @@ mod tests { } } + #[test] + fn ruby_error_contract_covers_every_predicate_once() { + let mut seen = ErrorPredicates::default(); + + for (role, priority) in [ + (ErrorPredicateRole::NativeKind, RUBY_ERROR_KIND_PRIORITY), + ( + ErrorPredicateRole::RequestDetail, + RUBY_REQUEST_DETAIL_PRIORITY, + ), + (ErrorPredicateRole::Diagnostic, RUBY_DIAGNOSTIC_PREDICATES), + ] { + for &predicate in priority { + assert_eq!(role, predicate.role()); + assert!( + !seen.contains(predicate), + "duplicate predicate: {predicate:?}" + ); + seen = seen.include_if(predicate, true); + } + } + + assert_eq!(ErrorPredicate::ALL.len(), seen.bits().count_ones() as usize); + + for &predicate in RUBY_DIAGNOSTIC_PREDICATES { + assert_eq!(None, predicates(&[predicate]).classifying_predicate()); + } + } + #[test] fn error_classification_separates_native_kinds_from_request_details() { let cases: &[(&[ErrorPredicate], Option)] = &[ @@ -512,6 +577,14 @@ mod tests { &[ErrorPredicate::Request, ErrorPredicate::ProxyConnect], Some(ErrorPredicate::ProxyConnect), ), + ( + &[ + ErrorPredicate::Request, + ErrorPredicate::Connect, + ErrorPredicate::ProxyConnect, + ], + Some(ErrorPredicate::ProxyConnect), + ), ( &[ErrorPredicate::Request, ErrorPredicate::Timeout], Some(ErrorPredicate::Timeout), diff --git a/test/error_hierarchy_test.rb b/test/error_hierarchy_test.rb index 9901e0c..d90ec02 100644 --- a/test/error_hierarchy_test.rb +++ b/test/error_hierarchy_test.rb @@ -73,14 +73,32 @@ def test_binding_generated_errors_have_no_native_predicates assert_empty active_native_predicates(error) end - def test_native_error_predicates_are_not_mutually_exclusive + def test_upstream_request_error_contract client = Wreq::Client.new(no_proxy: true) + with_invalid_tls_server do |url| + error = assert_raises(Wreq::ConnectError) do + client.get(url, timeout: 1) + end + + assert_equal %i[request? connect?], active_native_predicates(error) + end + + with_status_server(502) do |proxy| + error = assert_raises(Wreq::ProxyConnectError) do + Wreq.get( + "https://contract.invalid/", + proxy:, + timeout: 1 + ) + end + + assert_equal %i[request? proxy_connect?], active_native_predicates(error) + end + with_hanging_server do |url, _accepted| error = assert_raises(Wreq::TimeoutError) { client.get(url, timeout: 1) } - assert_predicate error, :timeout? - assert_predicate error, :request? assert_equal %i[timeout? request?], active_native_predicates(error) end end @@ -223,6 +241,31 @@ def closed_local_port server&.close end + def with_invalid_tls_server + server = TCPServer.new("127.0.0.1", 0) + thread = Thread.new do + socket = server.accept + header = socket.read(5) + payload_size = header.byteslice(3, 2).unpack1("n") + socket.read(payload_size) + + # Reply to the ClientHello with a fatal handshake_failure alert. + # https://www.rfc-editor.org/rfc/rfc8446#section-6 + socket.write [0x15, 0x03, 0x03, 0x00, 0x02, 0x02, 0x28].pack("C*") + socket.close_write + rescue IOError, SystemCallError + nil + ensure + socket&.close unless socket&.closed? + end + thread.report_on_exception = false + + yield "https://127.0.0.1:#{server.addr[1]}/" + ensure + server&.close unless server&.closed? + thread&.join(1) + end + def with_hanging_server server = TCPServer.new("127.0.0.1", 0) accepted = Queue.new @@ -250,6 +293,7 @@ def with_status_server(status, body: "") 204 => "No Content", 302 => "Found", 404 => "Not Found", + 502 => "Bad Gateway", 503 => "Service Unavailable" }.fetch(status) server = TCPServer.new("127.0.0.1", 0) From 35fb05d45803f7728c2ba61b775beac642501f25 Mon Sep 17 00:00:00 2001 From: gngpp Date: Mon, 3 Aug 2026 21:07:11 +0800 Subject: [PATCH 10/10] fix(error): decouple categories from native hierarchy --- lib/wreq_ruby/error.rb | 19 ++- src/error.rs | 361 ++++++++++++++++++++--------------------- 2 files changed, 186 insertions(+), 194 deletions(-) diff --git a/lib/wreq_ruby/error.rb b/lib/wreq_ruby/error.rb index cdc0e09..973fafd 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -10,15 +10,16 @@ module Wreq # be true. For example, a request timeout raises TimeoutError while both # `timeout?` and `request?` return true. # - # A native kind such as BodyError, TlsError, or StatusError takes precedence - # over details found in its cause chain. Native request errors are then - # classified as connection reset, timeout, proxy connect failure, - # destination connect failure, or RequestError, in that order. This order is - # defined by wreq-ruby and does not depend on the order of native checks. Use - # the predicates when code needs every native classification. Errors created - # by the binding itself return false for all of them. New native facts may be - # added as predicates without changing the exception class for existing - # failures. + # wreq-ruby records the native checks as facts, then chooses the exception + # class using its own rules. Native body, TLS, and status kinds take + # precedence over details found in their cause chains. The remaining errors + # are classified as connection reset, timeout, proxy connect failure, + # destination connect failure, or RequestError, in that order. These + # transport details do not depend on `request?` also being true. + # + # Use the predicates when code needs every native fact. Errors created by + # the binding return false for all of them. New facts may be exposed as + # predicates without changing the exception class for existing failures. # # @example Rescue any wreq-ruby runtime error # begin diff --git a/src/error.rs b/src/error.rs index bcec592..00553b6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -45,12 +45,11 @@ macro_rules! initialize_exception { }}; } -macro_rules! define_error_mapping { +macro_rules! define_native_error_predicates { ( $( $predicate:ident [$role:ident]: $native_method:ident as $ruby_method:ident - => $class:ident $(($ruby_name:literal))? ),+ $(,)? ) => { /// How a native predicate participates in the Ruby error contract. @@ -58,7 +57,7 @@ macro_rules! define_error_mapping { #[derive(Clone, Copy, Debug, PartialEq, Eq)] enum ErrorPredicateRole { NativeKind, - RequestDetail, + TransportDetail, Diagnostic, } @@ -78,7 +77,7 @@ macro_rules! define_error_mapping { 1 << (self as u8) } - /// Return whether this is a native kind or a request detail. + /// Return how this native fact participates in the Ruby contract. #[cfg(test)] const fn role(self) -> ErrorPredicateRole { match self { @@ -92,22 +91,11 @@ macro_rules! define_error_mapping { $(Self::$predicate => error.$native_method(),)+ } } - - /// Return the Ruby class represented by this predicate. - fn error_class(self) -> &'static Lazy { - match self { - $(Self::$predicate => &$class,)+ - } - } } const _: () = assert!(ErrorPredicate::ALL.len() <= ErrorPredicateBits::BITS as usize); - $( - $(define_exception!($class, $ruby_name, exception_runtime_error);)? - )+ - $( fn $native_method(rb_self: RObject) -> Result { error_has_predicate(rb_self, ErrorPredicate::$predicate) @@ -124,6 +112,33 @@ macro_rules! define_error_mapping { )+ Ok(()) } + }; +} + +macro_rules! define_ruby_error_categories { + ( + $( + $category:ident => $class:ident $(($ruby_name:literal))? + ),+ $(,)? + ) => { + /// Stable exception categories owned by the Ruby API. + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + enum RubyErrorCategory { + $($category),+ + } + + impl RubyErrorCategory { + /// Return the Ruby exception class for this stable category. + fn error_class(self) -> &'static Lazy { + match self { + $(Self::$category => &$class,)+ + } + } + } + + $( + $(define_exception!($class, $ruby_name, exception_runtime_error);)? + )+ /// Define and retain every mapped Ruby exception class. fn initialize_mapped_errors( @@ -142,49 +157,65 @@ macro_rules! define_error_mapping { } // wreq keeps its error kind private. Keep its mutually exclusive kind predicates -// separate from request details, which inspect the source chain and may overlap. +// separate from transport details, which inspect the source chain and may overlap. // Each entry maps the native method before `as` to the Ruby predicate after it. // Classification order is declared separately as part of the Ruby contract. -define_error_mapping! { - Builder [NativeKind]: is_builder as builder => BUILDER_ERROR("BuilderError"), - Body [NativeKind]: is_body as body => BODY_ERROR("BodyError"), - Tls [NativeKind]: is_tls as tls => TLS_ERROR("TlsError"), - Decode [NativeKind]: is_decode as decoding => DECODING_ERROR("DecodingError"), - Redirect [NativeKind]: is_redirect as redirect => REDIRECT_ERROR("RedirectError"), - Status [NativeKind]: is_status as status => STATUS_ERROR("StatusError"), - Upgrade [NativeKind]: is_upgrade as upgrade => WREQ_ERROR, - Request [NativeKind]: is_request as request => REQUEST_ERROR("RequestError"), - ConnectionReset [RequestDetail]: - is_connection_reset as connection_reset - => CONNECTION_RESET_ERROR("ConnectionResetError"), - Timeout [RequestDetail]: is_timeout as timeout => TIMEOUT_ERROR("TimeoutError"), - ProxyConnect [RequestDetail]: - is_proxy_connect as proxy_connect - => PROXY_CONNECT_ERROR("ProxyConnectError"), - Connect [RequestDetail]: is_connect as connect => CONNECT_ERROR("ConnectError"), -} - -/// Stable precedence for mutually exclusive native error kinds. +define_native_error_predicates! { + Builder [NativeKind]: is_builder as builder, + Body [NativeKind]: is_body as body, + Tls [NativeKind]: is_tls as tls, + Decode [NativeKind]: is_decode as decoding, + Redirect [NativeKind]: is_redirect as redirect, + Status [NativeKind]: is_status as status, + Upgrade [NativeKind]: is_upgrade as upgrade, + Request [NativeKind]: is_request as request, + ConnectionReset [TransportDetail]: + is_connection_reset as connection_reset, + Timeout [TransportDetail]: is_timeout as timeout, + ProxyConnect [TransportDetail]: + is_proxy_connect as proxy_connect, + Connect [TransportDetail]: is_connect as connect, +} + +define_ruby_error_categories! { + Base => WREQ_ERROR, + Builder => BUILDER_ERROR("BuilderError"), + Body => BODY_ERROR("BodyError"), + Tls => TLS_ERROR("TlsError"), + Decoding => DECODING_ERROR("DecodingError"), + Redirect => REDIRECT_ERROR("RedirectError"), + Status => STATUS_ERROR("StatusError"), + Request => REQUEST_ERROR("RequestError"), + ConnectionReset => CONNECTION_RESET_ERROR("ConnectionResetError"), + Timeout => TIMEOUT_ERROR("TimeoutError"), + ProxyConnect => PROXY_CONNECT_ERROR("ProxyConnectError"), + Connect => CONNECT_ERROR("ConnectError"), +} + +/// Stable mapping from native facts to Ruby exception categories. /// -/// This order belongs to the Ruby API. Reordering the mapping macro or changing -/// the order in which wreq evaluates predicates must not change rescue behavior. -const RUBY_ERROR_KIND_PRIORITY: &[ErrorPredicate] = &[ - ErrorPredicate::Builder, - ErrorPredicate::Body, - ErrorPredicate::Tls, - ErrorPredicate::Decode, - ErrorPredicate::Redirect, - ErrorPredicate::Status, - ErrorPredicate::Upgrade, - ErrorPredicate::Request, -]; - -/// Stable precedence for details that refine a native request error. -const RUBY_REQUEST_DETAIL_PRIORITY: &[ErrorPredicate] = &[ - ErrorPredicate::ConnectionReset, - ErrorPredicate::Timeout, - ErrorPredicate::ProxyConnect, - ErrorPredicate::Connect, +/// Non-request kinds keep their existing precedence. Source-chain details are +/// then classified independently of `is_request()`, with the generic request +/// category retained only as a fallback. This order belongs to the Ruby API. +const RUBY_ERROR_CLASSIFICATION: &[(ErrorPredicate, RubyErrorCategory)] = &[ + (ErrorPredicate::Builder, RubyErrorCategory::Builder), + (ErrorPredicate::Body, RubyErrorCategory::Body), + (ErrorPredicate::Tls, RubyErrorCategory::Tls), + (ErrorPredicate::Decode, RubyErrorCategory::Decoding), + (ErrorPredicate::Redirect, RubyErrorCategory::Redirect), + (ErrorPredicate::Status, RubyErrorCategory::Status), + (ErrorPredicate::Upgrade, RubyErrorCategory::Base), + ( + ErrorPredicate::ConnectionReset, + RubyErrorCategory::ConnectionReset, + ), + (ErrorPredicate::Timeout, RubyErrorCategory::Timeout), + ( + ErrorPredicate::ProxyConnect, + RubyErrorCategory::ProxyConnect, + ), + (ErrorPredicate::Connect, RubyErrorCategory::Connect), + (ErrorPredicate::Request, RubyErrorCategory::Request), ]; /// Native facts exposed to Ruby without changing the exception class. @@ -194,11 +225,11 @@ const RUBY_REQUEST_DETAIL_PRIORITY: &[ErrorPredicate] = &[ #[cfg(test)] const RUBY_DIAGNOSTIC_PREDICATES: &[ErrorPredicate] = &[]; -/// Native predicates retained after consuming a wreq error. +/// Native error facts retained after consuming a wreq error. #[derive(Clone, Copy, Default)] -struct ErrorPredicates(ErrorPredicateBits); +struct NativeErrorFacts(ErrorPredicateBits); -impl ErrorPredicates { +impl NativeErrorFacts { /// Restore predicates from compact Ruby metadata. const fn from_bits(bits: ErrorPredicateBits) -> Self { Self(bits) @@ -223,31 +254,16 @@ impl ErrorPredicates { self } - /// Return the first captured predicate in a binding-owned priority list. - fn first_present(self, priority: &[ErrorPredicate]) -> Option { - priority + /// Classify captured facts using the binding-owned Ruby contract. + fn ruby_category(self) -> RubyErrorCategory { + RUBY_ERROR_CLASSIFICATION .iter() - .copied() - .find(|predicate| self.contains(*predicate)) - } - - /// Select one Ruby exception class without treating all predicates as peers. - /// - /// Native kinds are mutually exclusive. Connection and timeout predicates - /// only refine the request kind because they inspect the error source chain. - fn classifying_predicate(self) -> Option { - let kind = self.first_present(RUBY_ERROR_KIND_PRIORITY)?; - - if kind == ErrorPredicate::Request { - self.first_present(RUBY_REQUEST_DETAIL_PRIORITY) - .or(Some(kind)) - } else { - Some(kind) - } + .find_map(|&(predicate, category)| self.contains(predicate).then_some(category)) + .unwrap_or(RubyErrorCategory::Base) } } -impl From<&wreq::Error> for ErrorPredicates { +impl From<&wreq::Error> for NativeErrorFacts { /// Snapshot every native predicate before consuming the wreq error. fn from(error: &wreq::Error) -> Self { ErrorPredicate::ALL @@ -263,7 +279,7 @@ impl From<&wreq::Error> for ErrorPredicates { struct ErrorMetadata<'a> { uri: Option<&'a str>, status: Option, - predicates: ErrorPredicates, + facts: NativeErrorFacts, } // Stable roots for native errors. @@ -413,19 +429,16 @@ pub fn type_error(ruby: &Ruby, message: impl Into>) -> MagnusE MagnusError::new(ruby.exception_type_error(), message) } -/// Select the most specific Ruby exception class for native predicates. -fn wreq_error_class(ruby: &Ruby, predicates: ErrorPredicates) -> ExceptionClass { - predicates.classifying_predicate().map_or_else( - || ruby.get_inner(&WREQ_ERROR), - |predicate| ruby.get_inner(predicate.error_class()), - ) +/// Select the Ruby exception class from the binding-owned category. +fn wreq_error_class(ruby: &Ruby, facts: NativeErrorFacts) -> ExceptionClass { + ruby.get_inner(facts.ruby_category().error_class()) } /// Read one native predicate from a Ruby error, defaulting to false. fn error_has_predicate(rb_self: RObject, predicate: ErrorPredicate) -> Result { rb_self .ivar_get::<_, Option>(ERROR_PREDICATES_IVAR) - .map(|bits| bits.is_some_and(|bits| ErrorPredicates::from_bits(bits).contains(predicate))) + .map(|bits| bits.is_some_and(|bits| NativeErrorFacts::from_bits(bits).contains(predicate))) } /// Construct a Ruby exception and attach captured native error metadata. @@ -437,7 +450,7 @@ fn error_with_metadata( ) -> MagnusError { match class.new_instance((message,)).and_then(|exception| { let object = RObject::try_convert(exception.as_value())?; - object.ivar_set(ERROR_PREDICATES_IVAR, metadata.predicates.bits())?; + object.ivar_set(ERROR_PREDICATES_IVAR, metadata.facts.bits())?; if let Some(uri) = metadata.uri { let uri = ruby.str_new(uri); @@ -458,8 +471,8 @@ fn error_with_metadata( /// Map [`wreq::Error`] to corresponding [`magnus::Error`]. pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { - let predicates = ErrorPredicates::from(&err); - let class = wreq_error_class(ruby, predicates); + let facts = NativeErrorFacts::from(&err); + let class = wreq_error_class(ruby, facts); let uri = err.uri().map(ToString::to_string); let status = err.status(); let message = err.without_uri().to_string(); @@ -471,7 +484,7 @@ pub fn wreq_error(ruby: &Ruby, err: wreq::Error) -> MagnusError { ErrorMetadata { uri: uri.as_deref(), status, - predicates, + facts, }, ) } @@ -500,35 +513,35 @@ pub fn include(ruby: &Ruby, gem_module: &RModule) -> Result<(), MagnusError> { #[cfg(test)] mod tests { use super::{ - ErrorPredicate, ErrorPredicateRole, ErrorPredicates, RUBY_DIAGNOSTIC_PREDICATES, - RUBY_ERROR_KIND_PRIORITY, RUBY_REQUEST_DETAIL_PRIORITY, + ErrorPredicate, ErrorPredicateRole, NativeErrorFacts, RUBY_DIAGNOSTIC_PREDICATES, + RUBY_ERROR_CLASSIFICATION, RubyErrorCategory, }; - fn predicates(entries: &[ErrorPredicate]) -> ErrorPredicates { + fn facts(entries: &[ErrorPredicate]) -> NativeErrorFacts { entries .iter() .copied() - .fold(ErrorPredicates::default(), |predicates, predicate| { - predicates.include_if(predicate, true) + .fold(NativeErrorFacts::default(), |facts, predicate| { + facts.include_if(predicate, true) }) } #[test] fn error_predicate_bits_are_unique_and_round_trip() { - let predicates = ErrorPredicate::ALL.iter().copied().fold( - ErrorPredicates::default(), - |predicates, predicate| { - assert!(!predicates.contains(predicate)); - predicates.include_if(predicate, true) + let facts = ErrorPredicate::ALL.iter().copied().fold( + NativeErrorFacts::default(), + |facts, predicate| { + assert!(!facts.contains(predicate)); + facts.include_if(predicate, true) }, ); assert_eq!( ErrorPredicate::ALL.len(), - predicates.bits().count_ones() as usize + facts.bits().count_ones() as usize ); - let restored = ErrorPredicates::from_bits(predicates.bits()); + let restored = NativeErrorFacts::from_bits(facts.bits()); for &predicate in ErrorPredicate::ALL { assert!(restored.contains(predicate)); } @@ -536,99 +549,77 @@ mod tests { #[test] fn ruby_error_contract_covers_every_predicate_once() { - let mut seen = ErrorPredicates::default(); - - for (role, priority) in [ - (ErrorPredicateRole::NativeKind, RUBY_ERROR_KIND_PRIORITY), - ( - ErrorPredicateRole::RequestDetail, - RUBY_REQUEST_DETAIL_PRIORITY, - ), - (ErrorPredicateRole::Diagnostic, RUBY_DIAGNOSTIC_PREDICATES), - ] { - for &predicate in priority { - assert_eq!(role, predicate.role()); - assert!( - !seen.contains(predicate), - "duplicate predicate: {predicate:?}" - ); - seen = seen.include_if(predicate, true); - } + let mut seen = NativeErrorFacts::default(); + + for &(predicate, _) in RUBY_ERROR_CLASSIFICATION { + assert_ne!(ErrorPredicateRole::Diagnostic, predicate.role()); + assert!( + !seen.contains(predicate), + "duplicate predicate: {predicate:?}" + ); + seen = seen.include_if(predicate, true); + } + + for &predicate in RUBY_DIAGNOSTIC_PREDICATES { + assert_eq!(ErrorPredicateRole::Diagnostic, predicate.role()); + assert!( + !seen.contains(predicate), + "duplicate predicate: {predicate:?}" + ); + seen = seen.include_if(predicate, true); } assert_eq!(ErrorPredicate::ALL.len(), seen.bits().count_ones() as usize); for &predicate in RUBY_DIAGNOSTIC_PREDICATES { - assert_eq!(None, predicates(&[predicate]).classifying_predicate()); + assert_eq!(RubyErrorCategory::Base, facts(&[predicate]).ruby_category()); } } #[test] - fn error_classification_separates_native_kinds_from_request_details() { - let cases: &[(&[ErrorPredicate], Option)] = &[ - (&[], None), - (&[ErrorPredicate::Upgrade], Some(ErrorPredicate::Upgrade)), - (&[ErrorPredicate::Request], Some(ErrorPredicate::Request)), - ( - &[ErrorPredicate::Request, ErrorPredicate::Connect], - Some(ErrorPredicate::Connect), - ), - ( - &[ErrorPredicate::Request, ErrorPredicate::ProxyConnect], - Some(ErrorPredicate::ProxyConnect), - ), - ( - &[ - ErrorPredicate::Request, - ErrorPredicate::Connect, - ErrorPredicate::ProxyConnect, - ], - Some(ErrorPredicate::ProxyConnect), - ), - ( - &[ErrorPredicate::Request, ErrorPredicate::Timeout], - Some(ErrorPredicate::Timeout), - ), - ( - &[ - ErrorPredicate::Request, - ErrorPredicate::Connect, - ErrorPredicate::Timeout, - ], - Some(ErrorPredicate::Timeout), - ), - ( - &[ - ErrorPredicate::Request, - ErrorPredicate::ProxyConnect, - ErrorPredicate::Timeout, - ], - Some(ErrorPredicate::Timeout), - ), - ( - &[ - ErrorPredicate::Request, - ErrorPredicate::ConnectionReset, - ErrorPredicate::Timeout, - ], - Some(ErrorPredicate::ConnectionReset), - ), - ( - &[ - ErrorPredicate::Body, - ErrorPredicate::Request, - ErrorPredicate::Timeout, - ], - Some(ErrorPredicate::Body), - ), - ]; - - for &(entries, expected) in cases { + fn ruby_error_classification_is_owned_by_the_binding() { + for &(predicate, category) in RUBY_ERROR_CLASSIFICATION { + assert_eq!(category, facts(&[predicate]).ruby_category()); + } + + assert_eq!(RubyErrorCategory::Base, facts(&[]).ruby_category()); + + for &(kind, kind_category) in RUBY_ERROR_CLASSIFICATION { + if kind.role() != ErrorPredicateRole::NativeKind || kind == ErrorPredicate::Request { + continue; + } + + for &(detail, _) in RUBY_ERROR_CLASSIFICATION { + if detail.role() == ErrorPredicateRole::TransportDetail { + assert_eq!( + kind_category, + facts(&[kind, detail]).ruby_category(), + "native kind {kind:?} must take precedence over {detail:?}" + ); + } + } + } + + for (index, &(detail, detail_category)) in RUBY_ERROR_CLASSIFICATION.iter().enumerate() { + if detail.role() != ErrorPredicateRole::TransportDetail { + continue; + } + assert_eq!( - expected, - predicates(entries).classifying_predicate(), - "predicates: {entries:?}" + detail_category, + facts(&[ErrorPredicate::Request, detail]).ruby_category(), + "transport detail {detail:?} must not depend on the request kind" ); + + for &(lower_priority, _) in &RUBY_ERROR_CLASSIFICATION[index + 1..] { + if lower_priority.role() == ErrorPredicateRole::TransportDetail { + assert_eq!( + detail_category, + facts(&[detail, lower_priority]).ruby_category(), + "transport detail {detail:?} must take precedence over {lower_priority:?}" + ); + } + } } } }