Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faraday tries to modify frozen URI (FrozenError) #1349

Open
sled opened this issue Dec 19, 2021 · 6 comments
Open

Faraday tries to modify frozen URI (FrozenError) #1349

sled opened this issue Dec 19, 2021 · 6 comments
Labels

Comments

@sled
Copy link
Contributor

sled commented Dec 19, 2021

Ruby Version = 3.0.2
Faraday Version = 1.8.0, 2.0.0

If a frozen URI is passed, Faraday tries to modify the URL and a FrozenError: can't modify frozen URI::HTTP: #<URI::HTTP http://example.org/api/v1> is raised.

The affected code sections

v1.8.0 https://github.com/lostisland/faraday/blob/v1.8.0/lib/faraday/connection.rb#L448-L459
v2.0.0 https://github.com/lostisland/faraday/blob/main/lib/faraday/connection.rb#L448-L459

Minimal code sample:

endpoint = URI('http://example.org/api/v1').freeze
connection = Faraday.new(endpoint)

ruby/3.0.2/lib/ruby/3.0.0/uri/generic.rb:780:in `set_path': can't modify frozen URI::HTTP: #<URI::HTTP http://example.org/api/v1> (FrozenError)
  from ruby/3.0.2/lib/ruby/3.0.0/uri/generic.rb:807:in `path='
  from ruby/3.0.2/lib/ruby/gems/3.0.0/gems/faraday-1.8.0/lib/faraday/connection.rb:455:in `path_prefix='
  from ruby/3.0.2/lib/ruby/gems/3.0.0/gems/faraday-1.8.0/lib/faraday/connection.rb:430:in `url_prefix='
  from ruby/3.0.2/lib/ruby/gems/3.0.0/gems/faraday-1.8.0/lib/faraday/connection.rb:84:in `initialize'
  from ruby/3.0.2/lib/ruby/gems/3.0.0/gems/faraday-1.8.0/lib/faraday.rb:112:in `new'
  from ruby/3.0.2/lib/ruby/gems/3.0.0/gems/faraday-1.8.0/lib/faraday.rb:112:in `new'

Is this behaviour intended?

@iMacTia
Copy link
Member

iMacTia commented Dec 20, 2021

Thanks for reporting this @sled, no I would say this is not intended! It seems like a simple overlooking.
When url is either a URI or string, we set url_prefix to it passing through Utils.URI. And if the URI is already a URI, then we just return the URI itself. The problem doesn't happen when url is a string because we build a new URI with it in that case.

I believe there should be a #dup somewhere, but I'm a bit unsure if this should happen in connection or in the Util class 🤔
Regardless, this is a good finding and I'd be happy to see it fixed with a PR if you have some spare time, I hope the info above helps!

@iMacTia iMacTia added the bug label Dec 20, 2021
@sled
Copy link
Contributor Author

sled commented Dec 24, 2021

@iMacTia The question is whether Utils.URI should mimic the original URI(...), because URI(...) does not duplicate when a URI instance is passed:

require 'uri'
uri1 = URI('https://example.org')
uri2 = URI(uri1)

uri1.object_id
# => 180
uri2.object_id
# => 180

@iMacTia
Copy link
Member

iMacTia commented Dec 26, 2021

That's a good point, and it also makes sense if we think about it. A simple library should not worry about this kind of stuff, stay simple and do the bare minimum. Shall we agree this should be a Faraday::Connection responsibility?

@sled
Copy link
Contributor Author

sled commented Dec 28, 2021

I found some inconsistency regarding the url_prefix:

It is documented as being a string here but it's expected to be URI-like here, here, here and here

Also, the scheme=, host=, and port= setters are delegated to the URI, allowing it to be modified. However these are setters which are called explicitly by the user, so if you passed in a frozen URL it could be argued that it is expected to fail.

That's not the case with path_prefix=, because it is called implicitly when setting the url_prefix=, modifying the path of the URI to ensure it begins with a slash.

I could imagine three options:

Keep the URI frozen

  • Remove the call to path_prefix= here
  • Simplify the path_prefix= setter to a simple assignment to uri.path= (maybe it could even be deprected and use a simple path= delegator to the URI like the already existing host= and port= delegators).
  • Add a path_prefix getter which appends a trailing slash if necessary, i.e. returns "/#{url_prefix.path}", not modifying the URI instance.
  • Use the path_prefix getter in build_exclusive_url instead of .path

This way, Faraday would work as expected, however explicit calls to URI setters like host=, port=, path_prefix= would fail.

Duplicating the URI

    def url_prefix=(url, encoder = nil)
      url = url.dup if url.frozen? && url.is_a?(URI)
      uri = @url_prefix = Utils.URI(url)
      self.path_prefix = uri.path
      # ...
    end

Converting to a string first

    def url_prefix=(url, encoder = nil)
      uri = @url_prefix = Utils.URI(url.to_s)
      self.path_prefix = uri.path
      # ...
    end

On a side note, the regex to check for a trailing slash here could be replaced with a simple base.path.end_with?('/') which probably is faster and more readable. Also here there's a direct call to URI.parse instead of Utils.URI(...) circumventing the custom URL parser.

@iMacTia
Copy link
Member

iMacTia commented Dec 29, 2021

@sled thanks for the deep analysis and for spotting these inconsistencies! I can only imagine this is the result of years of "patching" the code over and over again 😅.

I'd personally opt for a simple solution right now, either using #dup or .#to_s, despite the first solution potentially being better. The main reason being that i) this might create some issues with the "quick methods" (Faraday.get, Faraday.post, ...) and the fact that I'm planning a much deeper rewrite for Faraday 3.0.

So if that's OK with you, I'd go with one of the simple solutions (maybe the #to_s one, with a complimentary comment explaining why we're doing that) and instead create a new v3.0 issue to address this properly. It will be really useful to refer back to your comment when we get there.

On a side note, the regex to check for a trailing slash here could be replaced with a simple base.path.end_with?('/') which probably is faster and more readable. Also here there's a direct call to URI.parse instead of Utils.URI(...) circumventing the custom URL parser.

These sound like great improvements, I'd be happy to have them included in the PR 👍

@alexcwatt
Copy link

+1. I recently ran into a situation where passing a URI object into Faraday.new led to the URI being mutated, which I did not expect. My object wasn't frozen but it sounds like the same issue as this. It sounds like #dup or #to_s would solve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants