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

2.7.3 breaks compatibility with Addressable as URI parser #1484

Closed
pharmaceutics opened this issue Jan 20, 2023 · 5 comments · Fixed by #1485
Closed

2.7.3 breaks compatibility with Addressable as URI parser #1484

pharmaceutics opened this issue Jan 20, 2023 · 5 comments · Fixed by #1485

Comments

@pharmaceutics
Copy link

Basic Info

  • Faraday Version: 2.7.3
  • Ruby Version: 3.0.4

Issue description

Not sure if it's still officially supported, but in the past, Faraday allowed replacing the default URI parser with a custom one (like Addressable::URI#parse). It was working fine before 2.7.3, but due to changes in #1481 Addressable is no longer compatible.
The example below works fine with 2.7.2, but with 2.7.3 throws:

/usr/local/bundle/gems/faraday-2.7.3/lib/faraday/connection.rb:476:in `build_exclusive_url': undefined method `opaque' for #<Addressable::URI:0x550 URI:http://example.com> (NoMethodError)
	from /usr/local/bundle/gems/faraday-2.7.3/lib/faraday/rack_builder.rb:202:in `build_env'
	from /usr/local/bundle/gems/faraday-2.7.3/lib/faraday/rack_builder.rb:153:in `build_response'
	from /usr/local/bundle/gems/faraday-2.7.3/lib/faraday/connection.rb:444:in `run_request'
	from /usr/local/bundle/gems/faraday-2.7.3/lib/faraday/connection.rb:200:in `get'
	from /usr/local/bundle/gems/faraday-2.7.3/lib/faraday.rb:145:in `method_missing'
	from test2.rb:11:in `<main>'

Steps to reproduce

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'addressable', '2.8.1'
  gem 'faraday', '2.7.3'
end

Faraday::Utils.default_uri_parser = Addressable::URI

puts Faraday.get('http://example.com').status
@olleolleolle
Copy link
Member

Hm, would it be possible to use URI::Generic's https://rubyapi.org/2.5/o/uri/generic#method-i-check_opaque

@iMacTia
Copy link
Member

iMacTia commented Jan 20, 2023

Apologies about that @pharmaceutics.
I've checked the code and indeed Utils.URI can be configured to use other parsers as well, but in this scenario we need to call opaque which is not a very standard method.

I don't like this open-ended configurability, since we don't have a way to enforce that the configured parser returns an object that behaves exactly like a URI, but I'm happy to change this to bring back compatibility as it was before.

Just as a curiosity, why do you override the default_uri_parser?

iMacTia added a commit that referenced this issue Jan 20, 2023
Utils.URI can be configured to use a different parser and return an object that "behaves like a URI". However, `opaque` is not available in other objects (e.g. Addressable::URI), so in this instance we need to use URI.

Fixes #1484
iMacTia added a commit that referenced this issue Jan 20, 2023
Utils.URI can be configured to use a different parser and return an object that "behaves like a URI". However, `opaque` is not available in other objects (e.g. Addressable::URI), so in this instance we need to use URI.

Fixes #1484
@iMacTia
Copy link
Member

iMacTia commented Jan 20, 2023

@pharmaceutics PR with a fix is merged and released as v2.7.4 👍

I'd still be interested to hear about your use-case to use Addressable::URI as a parser, if possible 🙏

@pharmaceutics
Copy link
Author

pharmaceutics commented Jan 21, 2023

@iMacTia thanks a lot! The actual parser in the code looked like this

Faraday::Utils.default_uri_parser = -> (v) { Addressable::URI.parse(v).normalize }

and two use cases I see tested are handling IDNAs and path parts with invalid characters that both wouldn't normally work (for example, with Addressable https://はじめよう.みんな/some|path is automatically encoded as https://xn--p8j9a0d9c9a.xn--q9jyb4c/some%7Cpath)
But there's probably no real reason why the URL couldn't be normalized separately first, with Faraday getting an already encoded version that would work with the regular Kernel#URI

@iMacTia
Copy link
Member

iMacTia commented Jan 23, 2023

Thanks @pharmaceutics, it doesn't seem like an extremely useful feature and it does create some maintainance issues, so it's likely we'll remove it on the next major version.

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 a pull request may close this issue.

3 participants