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

Fix parsing of scheme that are invalid Ruby constant names #27

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

byroot
Copy link
Member

@byroot byroot commented Jul 27, 2021

This fixes a regression from #26

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

Some symbols such as -, + or . are valid inside URI schemes.
See: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml

@eregon @hsbt

@@ -91,7 +91,7 @@ def self.for(scheme, *arguments, default: Generic)
const_name = scheme.to_s.upcase

uri_class = INITIAL_SCHEMES[const_name]
if !uri_class && !const_name.empty? && Schemes.const_defined?(const_name, false)
if !uri_class && !const_name.empty? && /\A[A-Z][A-Z0-9_]+\z/.match?(const_name) && Schemes.const_defined?(const_name, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&& !const_name.empty? can be removed with this change.

/\A[A-Z][A-Z0-9_]+\z/.match?(const_name)

Should it be /\A[A-Z]\w*\z/.match?(const_name) rather (allow lowercase letters from the 2nd character, single upper character allowed)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, I shouldn't have implemented this this late. Will fix.

assert_equal 'ms-search', URI.parse('ms-search://localhost').scheme
assert_equal 'microsoft.windows.camera', URI.parse('microsoft.windows.camera://localhost').scheme
assert_equal 'coaps+ws', URI.parse('coaps+ws:localhost').scheme
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be interesting to add a test for register_scheme with a -, . or +.
Probably ends up in an exception, but better than e.g. allowing to register but then the class is not used when parsing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure if the intent was to have register_scheme be a public API or not. My priority was to fix the parsing of arbitrary schemes.

If register is public API, then I'd rather leave it to you to fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I"d consider it public API because it is documented in

uri/lib/uri.rb

Line 33 in 86de79c

# register_scheme 'RSYNC', RSYNC

@byroot
Copy link
Member Author

byroot commented Jul 28, 2021

@eregon should be good to go now. I'd rather handle testing register_scheme in another PR.

@eregon eregon merged commit bc47bf7 into ruby:master Jul 28, 2021
@eregon
Copy link
Member

eregon commented Jul 28, 2021

Thanks for the fix!

@eregon
Copy link
Member

eregon commented Jul 28, 2021

BTW, it's interesting that const_defined? raises NameError when it could return false.
Or that constant names are pretty strict (/\A[A-Z]\w*\z/), when method names are not (can include spaces etc).
But that's how it is in Ruby currently.

@byroot
Copy link
Member Author

byroot commented Jul 28, 2021

Thanks for the merge.

BTW, it's interesting that const_defined? raises NameError when it could return false.

I had the exact same thought. Not sure if worth changing upstream.

On another note, may I request for this changed to be pulled in ruby soonish? It's much harder to spot new breaking changes when we have existing failing tests.

matzbot pushed a commit to ruby/ruby that referenced this pull request Jul 28, 2021
@eregon
Copy link
Member

eregon commented Jul 28, 2021

On another note, may I request for this changed to be pulled in ruby soonish? It's much harder to spot new breaking changes when we have existing failing tests.

Done: ruby/ruby@59a65f2

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

Successfully merging this pull request may close these issues.

2 participants