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..174213a 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 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 # 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,59 @@ 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 + # @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 is_proxy_connect + # @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 - 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 @@ -122,22 +138,29 @@ 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. # - # 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 `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) # 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_connect?` remains true. # # @example Handle a proxy connection failure # begin @@ -145,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. # @@ -168,20 +191,25 @@ 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 # 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 +246,10 @@ class RedirectError < Error; end # Raised when a request operation exceeds its timeout. # + # 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) # begin diff --git a/src/error.rs b/src/error.rs index b98946e..243d395 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_connect + => PROXY_CONNECT_ERROR("ProxyConnectError"), + Connect [RequestDetail]: is_connect as connect => CONNECT_ERROR("ConnectError"), } /// 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_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 0e4c1d9..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 @@ -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? + connect? + proxy_connect? + connection_reset? + body? + tls? + decoding? + upgrade? ].freeze def test_regular_errors_share_stable_root @@ -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 @@ -57,9 +61,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 +74,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 +141,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 +207,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