retry middleware, exceptions option as String does not match on exception sub-classes

[jrochkind]

Basic Info

• Faraday Version: 1.7.0
• Ruby Version: 2.7.4

Issue description

The exceptions option is documented to work as a module/class (eg Faraday::Error), or as a string (eg "Faraday::Error").

If you pass it as the class object itself, it matches on all sub-classes. For instance, if you pass Faraday::Error, the retry will still happen if the exception raised were Faraday::ConnectionFailed or Faraday::TimeoutError, as these are both sub-classes of Faraday::Error. This is exactly what a rubyist would expect; it's how similar APIs (including ruby rescue itself) generally work.

However, if you pass the argument instead as a string, "Faraday::Error", it will only be considered matched if the exception raised is exactly a Faraday::Error, not a sub-class like Faraday::ConnectionFailed or Faraday::TimeoutError.

This is unexpected, that passing a string changes semantics like this. Either it's a bug that String argument works differently (I think it probably is?); or it should be documented that it works differently.

We can see why it works how it works in the implementation here:

faraday/lib/faraday/request/retry.rb

Line 187 in 506e97a

error.class.to_s == ex.to_s

If it is to work as expected, either that would need to be changed to something like error.class.ancestors.collect(&:to_s).include?(ex.to_s), or the code would just have to turn the string into the actual class/module it represents before just using is_a? as normal.

I could submit a PR if someone provides guidance as to preferred path.

[iMacTia]

Thank you for raising this @jrochkind and for doing some good research.

Although I think the latter solution ("the code would just have to turn the string into the actual class/module it represents before just using is_a? as normal") would be best, I believe the main purpose of the string option is to avoid cases where the Exception may not be initialised yet. This may particularly be an issue in Rails-like environments where there's heavy usage of autoloading.

But that could be fixed with an extra check on the the module actually being defined, which I still think it's a better solution than reimplementing traversing the ancestors array as Ruby already does inside is_a?.

I'd welcome a PR attempting that and introducing test coverage to show both options can now check sub-classes as well 👍

[jrochkind]

Thanks for the response!

I guess the way to turn a string like "Faraday::Error" into a module is: Kernel.const_get("Faraday::Error")

I'm not sure how to check if it's defined first... can't figure out a way to do that. I think maybe it'd have to be a rescue?

Hmm something like:

              if ex.is_a? Module
                error.is_a? ex
              else
                begin
                  error.class.to_s == Kernel.const_get(ex.to_s)
                rescue NameError
                end
              end    

As I think about it more, the version with the "ancestors" check seems maybe simpler and preferable? Unless maybe there's a performance problem?

One possible use case for the string version is avoiding problems when the class you want to mention is automatically reloaded per-request by Rails development-mode, so the module you captured at boot is no longer right?

Not sure what use case(s) motivated the original feature, but you're right that it is possible for the class not to exist, so I guess backwards compatibility alone would say that should not raise if it doesn't currently.

It doesn't look like there's currently any test for exceptions option as a string; but there is a basic test of the exceptions option.

faraday/spec/faraday/request/retry_spec.rb

Lines 30 to 34 in 506e97a

	context 'and this is passed as a custom exception' do
	let(:options) { [{ exceptions: StandardError }] }
	it { expect(times_called).to eq(3) }
	end

So seems straightforward to add an alternate with a string version; and maybe a test that a class name that doesn't exist doesn't raise, too?

I'll try to find time to make and submit a PR next week.

[iMacTia]

Using a rescue this can be pretty simple, but you still need to use is_a? after the conversion, otherwise you won't support subclasses. If you replace error.class.to_s == Kernel.const_get(ex.to_s) with error.is_a?(Kernel.const_get(ex.to_s)) then you can see there's a bit of repetition, so the whole thing could become:

unless ex.is_a?(Module)
  # Use Object instead of Kernel, this is also what active support uses internally in `#constantize` 
  ex = Object.const_get(ex.to_s) rescue nil

  # if the conversion fails, the error doesn't exist, so we can't possibly match it with anything
  return false unless ex
end

# If you reach this point, `ex` MUST be a Module/Class
error.is_a?(ex)

I'm not sure how to check if it's defined first

If you prefer avoid using rescue, you can use const_defined? which accepts a string.
The following should also work:

unless ex.is_a?(Module)
  # if the conversion fails, the error doesn't exist, so we can't possibly match it with anything
  return false unless Object.const_defined?(ex.to_s)
  
  # Use Object instead of Kernel, this is also what active support uses internally in `#constantize` 
  ex = Object.const_get(ex.to_s)
end

# If you reach this point, `ex` MUST be a Module/Class
error.is_a?(ex)

And the latter is much more performant (~4x) when it comes to missing constants:

# When the provided constant exists
       user     system      total        real
with rescue  0.040020   0.000116   0.040136 (  0.040287)
with defined?  0.049568   0.000060   0.049628 (  0.049735)

# When the provided constant does NOT exist
       user     system      total        real
with rescue  0.195727   0.015417   0.211144 (  0.211313)
with defined?  0.050076   0.000255   0.050331 (  0.050378)

So seems straightforward to add an alternate with a string version; and maybe a test that a class name that doesn't exist doesn't raise, too?

That would be great!

[jrochkind]

OK thanks for the tips! I'll see what I can do.

(I definitely avoid the one-liner rescue nil which rescues all StandardError, I'd only want to rescue NameError. But const_defined? seems preferable.)

[iMacTia]

@jrochkind just a quick note that your PR was successfully merged in main, but that's currently pointing to the next major 2.0 release. I just realised that in your issue description you mention v1.7. If you would like this change backported, you can cherry-pick your commit into a new PR against the 1.x branch, so that I can merge that in and cut a new release.

[jrochkind]

Ah, ok, thanks for the note! I will see about that maybe. Is there any general estimate of when 2.0 might drop, like even just is it "almost done" at this point, or "just beginning"?

[iMacTia]

We've got a project board to track progress, most of the heavy-lifting is done. The only things left are a ticket about timeout errors (which I'm still not 100% sure to include in this major release) and other minor decorative changes. So I'd say we're close, though I'm always struggling for time.
I could probably release it in less than a week if I could focus on it full time 😄

And just to be clear, I personally wouldn't push for back-porting the fix, I'm all for keeping it on v2.0 👍
I was simply raising this in case you needed it and couldn't upgrade. A good test might be to point to the main branch in your project and see what is breaking, we also have a handy UPGRADING.md file to help with the update 🙌