Skip to content

Conversation

@yykamei
Copy link
Contributor

@yykamei yykamei commented Aug 15, 2025

Description

I found that Response#to_hash might cause NoMethodError when it's initialized without env.

irb(main):001> require 'faraday'
=> true
irb(main):002> Faraday::Response.new.to_hash
/Users/yykamei/git/Fork/faraday/lib/faraday/response.rb:63:in 'Faraday::Response#to_hash': undefined method 'status' for nil (NoMethodError)

        status: env.status, body: env.body,
                   ^^^^^^^
	from (irb):2:in '<main>'
	from <internal:kernel>:168:in 'Kernel#loop'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/irb-1.14.3/exe/irb:9:in '<top (required)>'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/site_ruby/3.4.0/rubygems.rb:319:in 'Kernel#load'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/site_ruby/3.4.0/rubygems.rb:319:in 'Gem.activate_and_load_bin_path'
	from /Users/yykamei/.rbenv/versions/3.4.5/bin/irb:25:in '<top (required)>'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli/exec.rb:59:in 'Kernel.load'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli/exec.rb:59:in 'Bundler::CLI::Exec#kernel_load'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli/exec.rb:23:in 'Bundler::CLI::Exec#run'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli.rb:452:in 'Bundler::CLI#exec'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/vendor/thor/lib/thor/command.rb:28:in 'Bundler::Thor::Command#run'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in 'Bundler::Thor::Invocation#invoke_command'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/vendor/thor/lib/thor.rb:538:in 'Bundler::Thor.dispatch'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli.rb:35:in 'Bundler::CLI.dispatch'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/vendor/thor/lib/thor/base.rb:584:in 'Bundler::Thor::Base::ClassMethods#start'
	from /Users/yykamei/.rbenv/versions/3.4.5/lib/ruby/gems/3.4.0/gems/bundler-2.6.8/lib/bundler/cli.rb:29:in 'Bundler::CLI.start'
	... 6 levels...

In this case, #to_hash cannot return any data, so I added guard clause to return {} when response is not finished.

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • [ ] Documentation
    • I think the changes don't require documentation changes.

Add guard clause to return `{}` when response is not finished in order
to prevents potential errors when accessing response data `env` before
completion.
Comment on lines 62 to 63
return {} unless finished?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @yykamei, good catch. A more explicit alternative would be to reuse the status, body and headers methods already defined in this class and adding def url. It also gives us an explicit reference that env = nil by retaining the hash keys and the nil values and not only returning an empty hash. What do you think?

    def url
      finished? ? env.url : nil
    end
    def to_hash
      {
        status: status, body: body,
        response_headers: headers,
        url: url
      }
    end

Copy link
Contributor Author

@yykamei yykamei Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then I fix it and make the method return the data like this:

{
  status: nil,
  body: nil,
  response_headers: {},
  url: nil
}

Thank you for reviewing!

Copy link
Contributor Author

@yykamei yykamei Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the pull request with this commit: a857c26

@yykamei yykamei requested a review from rossme August 22, 2025 00:47
Copy link

@rossme rossme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏁

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing this @yykamei and thank you @rossme for the review!
I'm sorry it took me so long to get to this. I'm on it now and will try to get it merged and released ASAP 🙇

@iMacTia iMacTia merged commit edd8cc5 into lostisland:main Sep 28, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants