Failing test: Logging headers & errors fails when ConnectionFailed is raised

[eikes]

Description

I was not able to use faraday logging with the logging options headers: true, errors: true when the connection can not be estabilshed and a Faraday::ConnectionFailed error should be raised, because it raised a different error instead: NoMethodError: undefined method map' for nil:NilClass`

Here is the stacktrace:

         # ./lib/faraday/logging/formatter.rb:59:in `dump_headers'
         # ./lib/faraday/logging/formatter.rb:112:in `block in log_headers'
         # ./lib/faraday/logging/formatter.rb:113:in `public_send'
         # ./lib/faraday/logging/formatter.rb:113:in `log_headers'
         # ./lib/faraday/logging/formatter.rb:46:in `exception'
         # ./lib/faraday/response/logger.rb:31:in `on_error'
         # ./lib/faraday/middleware.rb:21:in `rescue in call'
         # ./lib/faraday/middleware.rb:15:in `call'
         # ./lib/faraday/response/logger.rb:23:in `call'
         # ./lib/faraday/rack_builder.rb:153:in `build_response'
         # ./lib/faraday/connection.rb:444:in `run_request'
         # ./lib/faraday/connection.rb:200:in `get'

The reason this happens is that the Faraday::Logging::Formatter#exception method tries to log the headers of the exception, but there aren't any. #log_headers is still called though, because exc.respond_to?(:response_headers) && log_headers?(:error) is true, as the Faraday::Error class implements the #response_headers method. Again, the @response of the exc instance is nil, so when the dump_headers method is finally called, it is called with nil. And there it fails because it can't call map on nil.

There are various ways to solve this, but the simplest would be to check for nil in the dump_headers method:

      def dump_headers(headers)
        return if headers.nil?
        # or:
        return unless headers.respond_to?(:map)
        headers.map { |k, v| "#{k}: #{v.inspect}" }.join("\n")
      end

You could also catch this in the exception method, but I think catching it in the dump_headers method is more robust:

      def exception(exc)
       #...
        log_headers('error', exc.response_headers) if exc.respond_to?(:response_headers) && !exc.response_headers.nil? && log_headers?(:error)
       #...
      end

[iMacTia]

Thank you for opening this @eikes, I love that you provided a failing test to demonstrate the issue.
I'm trying to understand if this is somehow related to this recent change, but I'm not sure there's any connection at this point.

My preference would also be to add a return if headers.nil? guard to the dump_headers method 👍

[eikes]

@iMacTia Thank you for the kind and immediate feedback! I made the change and the test is now passing... 😄

[iMacTia]

Thank you again @eikes, it's a pleasure to review well-thought PRs like this ❤️