Fix instances of frozen empty string literals

[BobbyMcWho]

Same fix as #1039, this should go into master as well for 1.0

[BobbyMcWho]

@iMacTia we'll likely still want to merge this

[iMacTia]

Sorry @BobbyMcWho I haven't had the time to look into this, but it seems like we're making the response body mutable. I'm not sure on what's the rationale behind it?
It would be good to see a use-case on why this would be required

[BobbyMcWho]

This was one of the many issues that presented itself when that 0.16.0 rollout was released. There are other projects that want to do things like force the encoding which were broken by introducing these non mutable bodies

[BobbyMcWho]

Example: https://github.com/fastlane/fastlane/pull/15403/files#diff-a9086da2d7e8a66b647fdabdb0a7cca7L811

[iMacTia]

Ah I see your point, thanks for clarifying.
My view though is that in case of a v1.0 release, this kind of backwards-incompatible things are to be expected and should not be an issue.

From an application point of view, I think having the body immutable is actually the correct behaviour in this case (faster performance, more security).

But I'm happy to take in different point of views

[BobbyMcWho]

Is the body always immutable when returned from Faraday or is it only immutable in the case when the body is this initial empty string?

[iMacTia]

I'd personally expect it to always be immutable, at least it would be consistent and as I said 99% of times you probably don't need to do any change to it (and you can always override it on that remaining 1%) and would keep performance faster

[BobbyMcWho]

is the body always immutable when we parse the response today?

[iMacTia]

You mean in 0.17.x? No it's not, but that's the old standard behaviour.
The mistake was to make it immutable in a minor release (0.16.0), but pushing this change into a major release I think it's acceptable. That would mean gems like fastlane would need to replace the body instead of appending to it (or mutating it, not sure what it does 😅).

My point is that I don't think it's worth keeping backwards compatibility in this case as you have performance on the other side. If we don't change this behaviour in a major release, we won't be able to change it anymore

[BobbyMcWho]

@iMacTia here's a script that demonstrates what I'm alluding to:

require 'bundler/inline'

gemfile do 
  source 'https://www.rubygems.org'
  gem 'faraday', '~> 1.0-rc1'
  gem 'minitest'
end

require 'minitest/autorun'
require 'faraday'

class FaradayTest < Minitest::Test
  def test_faraday_version
    assert Gem::Version.new(Faraday::VERSION) == Gem::Version.new('1.0-rc1') # pass
  end

  def test_faraday_frozen_body
    assert Faraday.get('http://www.example.com').body.frozen? # fail
  end
end

The default behavior is that the response body is not frozen, it is only frozen in these cases where we default the response body to an empty string.

Edit: I was testing the whole response and not just the body 🤦‍♂️

Double edit: Testing if the body is frozen, my original claim stands that the body is not frozen.

[technoweenie]

Is the body always immutable when returned from Faraday

This depends on the adapter and underlying gem. For example, the Excon adapter simply passes the response body to Faraday::Adapter#save_response. That's where Faraday could freeze the response body, if we wanted. I don't think it's necessary though.

faraday/lib/faraday/adapter/excon.rb

Lines 29 to 32 in cccc145

	resp = conn.request(req_opts)
	save_response(env, resp.status.to_i, resp.body, resp.headers,
	resp.reason_phrase)

faraday/lib/faraday/adapter.rb

Lines 56 to 59 in cccc145

	def save_response(env, status, body, headers = nil, reason_phrase = nil)
	env.status = status
	env.body = body
	env.reason_phrase = reason_phrase&.to_s&.strip

[BobbyMcWho]

In that case I definitely think the default empty string body should be mutable like my PR suggests

[iMacTia]

Agree we should be consistent on it, and currently we're relying on underlying adapters by just returning what they return us. Eventually one day everyone will move towards frozen strings as that's supposed to be the standard as older Ruby versions are abandoned, but we're not there just yet.
@technoweenie @olleolleolle I see really only 2 options here:

1. We explicitly freeze the body from adapters libraries so we get consistent behaviour across the line
2. When we set the body explicitly, we make it mutable to be consistent with adapters.

[olleolleolle]

@iMacTia If we are to do anything to the body returned, I think it is to freeze it (or do nothing).

[technoweenie]

@BobbyMcWho: There are other projects that want to do things like force the encoding which were broken by introducing these non mutable bodies

I'm down with frozen strings, but I think this is a really important point.

>> "".freeze.force_encoding("UTF-8")
Traceback (most recent call last):
        3: from /Users/rick/.rbenv/versions/2.5.5/bin/irb:11:in `<main>'
        2: from (irb):10
        1: from (irb):10:in `force_encoding'
FrozenError (can't modify frozen String)

By freezing empty strings Faraday, we place the burden on our users to dup response bodies that need their encoding changed. Sometimes it's necessary for reasons beyond the app developer's control. For example, it's called in a custom Faraday middleware in fastlane because of a bug.

I'm going with option 2, that we merge this PR and keep those response bodies mutable. Response bodies are used very differently than hash keys or constants, so I don't think the memory/performance profile will change much. While I think there's some merit in freezing the strings to maintain the integrity of the response body, I don't think it's worth breaking #force_encoding.

[olleolleolle]

That's a good reason, I retract my note.

[iMacTia]

Thanks @BobbyMcWho for the contribution and sorry for the loooong discussion (plus the extra issues with Github Actions)

[BobbyMcWho]

No need to apologize, I can definitely appreciate being thorough and weighing the options, especially with a library as widely used as Faraday.