Skip to content

Fix Response#to_hash to return empty hash when not finished#1639

Merged
iMacTia merged 3 commits into
lostisland:mainfrom
yykamei:finished_to_hash
Sep 28, 2025
Merged

Fix Response#to_hash to return empty hash when not finished#1639
iMacTia merged 3 commits into
lostisland:mainfrom
yykamei:finished_to_hash

Conversation

@yykamei

@yykamei yykamei commented Aug 15, 2025

Copy link
Copy Markdown
Contributor

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 thread lib/faraday/response.rb Outdated
Comment on lines +62 to +63
return {} unless finished?

Copy link
Copy Markdown

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

@yykamei yykamei Aug 22, 2025

Copy link
Copy Markdown
Contributor Author

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!

@yykamei yykamei Aug 22, 2025

Copy link
Copy Markdown
Contributor Author

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

@rossme rossme left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🏁

@iMacTia iMacTia left a comment

Copy link
Copy Markdown
Member

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
MitchLewis930 added a commit to Signal65/faraday-Greptile that referenced this pull request Jan 21, 2026
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