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.

Sign up for GitHub

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

Jump to bottom

Add proper Ractor support to URI #26

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Add proper Ractor support to URI #26

merged 3 commits into from
Jun 25, 2021

Conversation

eregon
Copy link
Member

@eregon eregon commented Jun 25, 2021

This avoids the drawbacks/hacks of both #22 and #15.

It uses a module to map scheme names to scheme classes, which also works with Ractor.

@eregon
Revert "Fix to support Ruby 3.0 Ractor"
b959da2 
* This reverts commit 1faa4fd.
* It has too many problems, see ruby#22 for discussion.
@eregon
Add proper Ractor support to URI
883567f 
* Using a module to map scheme name to scheme class, which also works with Ractor.
* No constant redefinition, no ObjectSpace, still fast lookup for initial schemes.
This was referenced Jun 25, 2021
Merged
Closed
@hsbt
Copy link
Member

hsbt commented Jun 25, 2021

Can you pick #25 with this pull-request?

@eregon
Copy link
Member Author

eregon commented Jun 25, 2021

@hsbt Done!

@eregon eregon mentioned this pull request Jun 25, 2021
Closed
@eregon
Copy link
Member Author

eregon commented Jun 25, 2021

@hsbt OK to merge this? CI is green and it fixes the bug found in #25 + the other advantages.

@hsbt
Copy link
Member

hsbt commented Jun 25, 2021

👍 Thanks!

@hsbt hsbt merged commit cdfb3f5 into ruby:master Jun 25, 2021
@casperisfine casperisfine mentioned this pull request Jul 27, 2021
Merged
byroot added a commit to rails/rails that referenced this pull request Jul 27, 2021
@byroot
Update globalid to 0.5.1 for ruby-head compatibility
bbaf40b 
ruby/uri#26 was recently pulled
in ruby-head, and it breaks globalid < 0.5.
@byroot
Copy link
Member

byroot commented Jul 27, 2021 •
edited
Loading

This seem to break support of schemes that aren't valid ruby constants, e.g:

Error:
ActiveRecord::ConnectionAdapters::MergeAndResolveDefaultUrlConfigTest#test_url_with_hyphenated_scheme:
NameError: wrong constant name IBM-DB
 
    if !uri_class && !const_name.empty? && Schemes.const_defined?(const_name, false)
                                                  ^^^^^^^^^^^^^^^
    /usr/local/lib/ruby/3.1.0/uri/common.rb:94:in `const_defined?'
    /usr/local/lib/ruby/3.1.0/uri/common.rb:94:in `for'
    /usr/local/lib/ruby/3.1.0/uri/rfc2396_parser.rb:210:in `parse'
    /rails/activerecord/lib/active_record/database_configurations/connection_url_resolver.rb:25:in `initialize'
    /rails/activerecord/lib/active_record/database_configurations/url_config.rb:48:in `new'
    /rails/activerecord/lib/active_record/database_configurations/url_config.rb:48:in `build_url_hash'
    /rails/activerecord/lib/active_record/database_configurations/url_config.rb:38:in `initialize'
    /rails/activerecord/lib/active_record/database_configurations.rb:262:in `new'
    /rails/activerecord/lib/active_record/database_configurations.rb:262:in `environment_url_config'
    /rails/activerecord/lib/active_record/database_configurations.rb:253:in `block in merge_db_environment_variables'
    /rails/activerecord/lib/active_record/database_configurations.rb:250:in `map'
    /rails/activerecord/lib/active_record/database_configurations.rb:250:in `merge_db_environment_variables'
    /rails/activerecord/lib/active_record/database_configurations.rb:180:in `build_configs'
    /rails/activerecord/lib/active_record/database_configurations.rb:19:in `initialize'
    /rails/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb:26:in `new'
    /rails/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb:26:in `resolve_db_config'
    /rails/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb:157:in `test_url_with_hyphenated_scheme'
 
rails test rails/activerecord/test/cases/connection_adapters/merge_and_resolve_default_url_config_test.rb:154

https://buildkite.com/rails/rails/builds/79689#6a1b7cfc-fa17-4752-bc27-62158a228e79/1018-1029

@byroot
Copy link
Member

byroot commented Jul 27, 2021

-, . and + are symbols that can be found in valid registered schemes: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

@byroot byroot mentioned this pull request Jul 27, 2021
Merged
smortex added a commit to riemann/riemann-tools that referenced this pull request Feb 13, 2023
@smortex
Fix required version for URI.register_scheme
51edd0e 
URI.register_scheme was added in ruby/uri#26 and
is part of `uri` 0.11.0.

URI 0.11.0 was imported in Ruby in
ruby/ruby@8ef125c
and is part of Ruby 3.1.0+.
smortex added a commit to riemann/riemann-tools that referenced this pull request Feb 13, 2023
@smortex
Fix required version for URI.register_scheme
bdeee8a 
URI.register_scheme was added in ruby/uri#26 and
is part of `uri` 0.11.0.

URI 0.11.0 was imported in Ruby in
ruby/ruby@8ef125c
and is part of Ruby 3.1.0+.

The CI did not catch it because `3.0` in YAML == `3` and `Ruby 3` is
`Ruby 3.2`.
maxlazio pushed a commit to gitlabhq/omnibus-gitlab that referenced this pull request May 15, 2023
@stanhu @balasankarc
Update Redis URL implementation to work with Ruby 3.1+
5df3c4d 
ruby/uri#26 refactored how schemes are
registered in the `uri` gem. Ruby 3.1.4 - 3.2.2 ships with uri
v0.12.1, which has this change.

Changelog: fixed
rtokarek-fastly added a commit to rtokarek-fastly/net-scp that referenced this pull request Jun 21, 2024
@rtokarek-fastly
Use ruby 3.1's uri's register_scheme if available
799bceb 
Ruby 3.1 adopted uri 0.11.0. That version changes how schemes can be registered. There is now a register_scheme method.

ruby/uri#26 as part of https://github.com/ruby/uri/releases/tag/v0.11.0

This commit supports both ways and works with the new method if available. The old way errors out when run  on uri>=0.11.0/ruby>=3.1.
@rtokarek-fastly rtokarek-fastly mentioned this pull request Jun 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eregon @hsbt @byroot