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

Any version after 2.8.1 causes errors in our test suite coming from addressable. #506

Closed
danielnolan opened this issue Apr 17, 2023 · 8 comments
Labels

Comments

@danielnolan
Copy link

danielnolan commented Apr 17, 2023

The change in 2.8.2 that sets the query to NONE seems to be causing issues with postrank-uri gem in our app. Any version of addressable after 2.8.1 and we start seeing these failures.

# --- Caused by: ---
          # TypeError:
          #   Can't convert Object into String.
          #   /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:1638:in `query='
# /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:1638:in `query='
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:853:in `block in initialize'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:2394:in `defer_validation'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:842:in `initialize'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:2170:in `new'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.4/lib/addressable/uri.rb:2170:in `normalize'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/util/uri.rb:20:in `block in <class:URI>'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/util/uri.rb:33:in `normalize_uri'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/request_signature.rb:10:in `initialize'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:267:in `new'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:267:in `request_signature_from_request'
          # /Users/danielnolan/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/webmock-3.18.1/lib/webmock/http_lib_adapters/net_http.rb:70:in `request'
@dentarg
Copy link
Collaborator

dentarg commented Apr 17, 2023

Can you post code that reproduce the problem?

@rickyc
Copy link

rickyc commented Apr 17, 2023

I was able to produce the error in Rails console using this code.

#  RestClient.version
=> "2.0.2"

RestClient.get("google.com")
TypeError: Can't convert Object into String.
from .gem/ruby/3.1.3/gems/addressable-2.8.4/lib/addressable/uri.rb:1636:in `query='

Can confirm switching addressable to version 2.8.1 fixes the issue.

[1] pry(main)> RestClient.get("google.com")
=> <RestClient::Response 200 "<!doctype h...">
[2] pry(main)> => <RestClient::Response 200 "<!doctype h...">

@danielnolan
Copy link
Author

It seems to be any rails project that has this gem included https://github.com/postrank-labs/postrank-uri
here is an app to reproduce https://github.com/danielnolan/addressable_bug all you should have to do is bundle and run rspec

@danielnolan danielnolan changed the title Any version after 2.8.1 causes errors in our test suite coming from webmock / adressable. Any version after 2.8.1 causes errors in our test suite coming from addressable. Apr 17, 2023
@dentarg
Copy link
Collaborator

dentarg commented Apr 17, 2023

Please provide code using only Addressable that reproduces the issue, thanks

@danielnolan
Copy link
Author

danielnolan commented Apr 17, 2023

@dentarg Not sure I can provide code that uses only Addressable and reproduces the issue, but going from 2.8.1 to 2.8.2 or greater shouldn't introduce breaking changes in gems that depend on addressable right? I would maybe expect breaking changes going from 2.8.x to 2.9, or 2.8 to 3.x, but not 2.8.1 to 2.8.2, as patch versions should be backwards compatible.

@dentarg
Copy link
Collaborator

dentarg commented Apr 18, 2023

There was no intention to introduce breaking changes in a patch version. This looks like a bug. In order to address the bug we need to know how to reproduce it.

Can you or someone else look into what the gems in the Rails repro are doing with addressable that causes this?

@danielnolan
Copy link
Author

Well after looking into the postrank-uri gem source it looks like it monkey patches the domain and normalized_query methods of the Addressable::URI class. So I'm not sure it actually is a bug in Addressable.
https://github.com/postrank-labs/postrank-uri/blob/master/lib/postrank-uri.rb#L8-L31

@dentarg
Copy link
Collaborator

dentarg commented Apr 19, 2023

I was able to play with this a bit little bit myself:

Addressable 2.8.2

$ ruby -rbundler/inline -e 'gemfile do; source "https://rubygems.org"; gem "postrank-uri"; gem "addressable", "2.8.2"; end; p Addressable::URI.parse("google.com").normalize'
/Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:1636:in `query=': Can't convert Object into String. (TypeError)

        raise TypeError, "Can't convert #{new_query.class} into String."
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	from /Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:851:in `block in initialize'
	from /Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:2392:in `defer_validation'
	from /Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:840:in `initialize'
	from /Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:2168:in `new'
	from /Users/dentarg/.arm64_rubies/3.2.2/lib/ruby/gems/3.2.0/gems/addressable-2.8.2/lib/addressable/uri.rb:2168:in `normalize'
	from -e:1:in `<main>'

Addressable 2.8.1

$ ruby -rbundler/inline -e 'gemfile do; source "https://rubygems.org"; gem "postrank-uri"; gem "addressable", "2.8.1"; end; p Addressable::URI.parse("google.com").normalize'
#<Addressable::URI:0x3d4 URI:google.com>

but yeah, that monkey patch in postrank-uri is super old, soon 12 years, postrank-labs/postrank-uri@110ed0b, and should probably be removed, or at least adjusted, I opened postrank-labs/postrank-uri#49

vasconsaurus added a commit to meedan/pender that referenced this issue Jul 18, 2023
There seems to be a super old monkey patch in postrank that messes
with the normalize method, starting at 2.8.2 version. Until postrank
fixes that we can only go up to 2.8.1

relevant links:
sporkmonger/addressable#513
sporkmonger/addressable#506
postrank-labs/postrank-uri#49
vasconsaurus added a commit to meedan/pender that referenced this issue Jul 19, 2023
* update rails to 6.1.7.4 while keeping sidekiq under 7

delayed extensions was removed in sidekiq 7 and that breaks things
for us
https://github.com/sidekiq/sidekiq/blob/main/Changes.md#640

* downgrade addressable to 2.8.1

There seems to be a super old monkey patch in postrank that messes
with the normalize method, starting at 2.8.2 version. Until postrank
fixes that we can only go up to 2.8.1

relevant links:
sporkmonger/addressable#513
sporkmonger/addressable#506
postrank-labs/postrank-uri#49
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