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 c98cd3e..4a7be17 100644 --- a/lib/wreq_ruby/error.rb +++ b/lib/wreq_ruby/error.rb @@ -5,16 +5,24 @@ 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 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 + # 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 # 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. @@ -32,51 +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 @@ -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 `connection?` 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 `proxy_connection?` 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 `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 # client = Wreq::Client.new(timeout: 1) # begin diff --git a/src/error.rs b/src/error.rs index b98946e..1f533d6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -46,30 +46,51 @@ 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]: + $native_method:ident as $ruby_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) -> ErrorPredicateBits { 1 << (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 { - $(Self::$predicate => error.$method(),)+ + $(Self::$predicate => error.$native_method(),)+ } } - /// 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,)+ @@ -78,22 +99,25 @@ macro_rules! define_error_mapping { } const _: () = - assert!(ErrorPredicate::CLASSIFICATION_ORDER.len() <= ErrorPredicateBits::BITS as usize); + assert!(ErrorPredicate::ALL.len() <= ErrorPredicateBits::BITS as usize); $( $(define_exception!($class, $ruby_name, exception_runtime_error);)? )+ $( - 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(()) } @@ -114,20 +138,27 @@ 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. +// 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: 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 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_connection + => PROXY_CONNECTION_ERROR("ProxyConnectionError"), + Connect [RequestDetail]: is_connect as connection => CONNECTION_ERROR("ConnectionError"), } /// Native predicates retained after consuming a wreq error. @@ -158,12 +189,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| { @@ -328,13 +382,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. @@ -417,9 +468,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)); @@ -428,13 +488,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..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 @@ -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_predicate error, :timeout? + assert_predicate error, :request? + assert_equal %i[timeout? request?], active_native_predicates(error) end end @@ -134,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" @@ -200,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