search

ruby / uri

URI is a module providing classes to handle Uniform Resource Identifiers
https://ruby.github.io/uri/
Other
79 stars 42 forks source link

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

Closed byroot closed 3 years ago

byroot commented 3 years ago

This fixes a regression from https://github.com/ruby/uri/pull/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

byroot commented 3 years ago

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

eregon commented 3 years ago

Thanks for the fix!

eregon commented 3 years ago

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 commented 3 years ago

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.

eregon commented 3 years ago

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: https://github.com/ruby/ruby/commit/59a65f2d2402e0d34d9232236f152d62e74f9483