Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Convert int, float, and bools from ENV['DATABASE_URL'] query args #9041

Merged
merged 1 commit into from

4 participants

@sodabrew

Previous title: SQLite3 busy_timeout function requires an int argument

This resolves an exception seen when calling rake db:create with a DATABASE_URL and a timeout argument:

DATABASE_URL="sqlite3://localhost/db/test.sqlite3?timeout=500&pool=5" bundle exec rake db:create
can't convert String into Integer
gems/activerecord-3.2.9/lib/active_record/connection_adapters/sqlite3_adapter.rb:31:in `busy_timeout'
...
Couldn't create database for {"adapter"=>"sqlite3", "timeout"=>"500", "database"=>"db/test.sqlite3", "host"=>"localhost", "pool"=>"5"}
@carlosantoniodasilva

Any possibility of adding a test for it?

@sodabrew

Been thinking about it all morning! Actually I might suggest moving the fix to the DATABASE_URL parsing function, and having it behave a little more like YAML: if one of the "query arguments" is all numeric, then cast it to a numeric type. That'd be more consistent through the system. Given these inputs, we get different outputs right now:

test:
  adapter: sqlite3
  database: test_db.sqlite3
  timeout: 500
  pool: 5

becomes

{"adapter"=>"sqlite3",
 "timeout"=>500,
 "database"=>"db/test.sqlite3",
 "host"=>"localhost",
 "pool"=>5}

but with a DATABASE_URL, all of the arguments become strings:

DATABASE_URL="sqlite3://localhost/db/test.sqlite3?timeout=500&pool=5"
{"adapter"=>"sqlite3",
 "timeout"=>"500",
 "database"=>"db/test.sqlite3",
 "host"=>"localhost",
 "pool"=>"5"}
@sodabrew

Ok! Ready for review again. I changed my approach to convert numeric types in the query string of the DATABASE_URL, and added and updated the connection spec tests to verify this.

...ecord/connection_adapters/connection_specification.rb
@@ -79,6 +79,8 @@ def connection_url_to_hash(url) # :nodoc:
options = Hash[config.query.split("&").map{ |pair| pair.split("=") }].symbolize_keys
spec.merge!(options)
end
+ # If anything looks numeric, make it numeric (e.g. port number, timeout values, etc.)
+ spec.map { |key,value| spec[key] = (Integer(value) rescue Float(value) rescue value) }
@rafaelfranca Owner

I think is better to check if the value can be an integer or float using a regexp

Is there an available regexp or activesupport is_numeric for this? I prefer not to embed a local magic regex here if I can instead leverage something built in to Ruby or Rails.

@rafaelfranca Owner

I don't remember, but you can always put the regexp in an private method

If you insist, but I think this way of using Ruby native classes is much cleaner.

@rafaelfranca Owner

The problem is using rescue to control flow. If we could use the native classes without rescue, so we should use they

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sodabrew

Branch updated! I didn't realize that rescue as control flow was against rails code style, thanks for pinpointing the issue for me.

@sodabrew

Second commit handles true / false as well. I can squash, drop, or change these as needed :octocat:

@rafaelfranca

Great! Could you add a CHANGELOG entry?

@sodabrew

Done! Squash all three together?

@rafaelfranca

Yes please. It needs a rebase too

@sodabrew sodabrew DATABASE_URL parsing should turn numeric strings into numeric types, and
the strings true and false into boolean types, in order to match how
YAML would parse the same values from database.yml and prevent
unexpected type errors in the database adapters.
4b005fb
@sodabrew

Rebased and squashed!

@rafaelfranca rafaelfranca merged commit 45883a3 into rails:master
@carlosantoniodasilva carlosantoniodasilva commented on the diff
...ecord/connection_adapters/connection_specification.rb
@@ -77,6 +81,21 @@ def connection_url_to_hash(url) # :nodoc:
spec.map { |key,value| spec[key] = uri_parser.unescape(value) if value.is_a?(String) }
if config.query
options = Hash[config.query.split("&").map{ |pair| pair.split("=") }].symbolize_keys
+ # If anything looks numeric, make it numeric (e.g. pool count, timeout values, etc.)
+ options.map do |key,value|

This could be each, since the return value is not used.

@rafaelfranca Owner

Will fix later. Thanks

@rafaelfranca Owner

Done 2b60a6b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca

Thank you

@sodabrew sodabrew deleted the sodabrew:patch-1 branch
@jeremyevans

This way of handling type conversions should be considered a design flaw, since you don't know how the options will be used later. Consider the case where a database option must be a string, and the string is allowed to contain one of the formats you are automatically converting (e.g. mysql://host/database?username=123456)? If specific options require a specific type (e.g. timeout must be an integer), that specific option handling should do the type conversion.

This commit just trades one set of type errors for a different set of type errors. Granted, the set of type errors it fixes in real applications may be greater than the set of type errors it causes in real applications, but it's still a poor way to handle things.

The comparison to YAML is invalid, since YAML has types and can differentiate between integers, floats, boolean values and strings.

Yes, this totally make sense. I'll investigate this better.

Thank you @jeremyevans

@jeremyevans when you use numbers or bools in string contexts, they evaluate to strings of those numbers and bools. e.g. false.to_s is "false", 1000.to_s is "1000", and so on. Otherwise finding the places where a number or a bool are expected is playing whack-a-mole with database adapters and their independently maintained gems. I think the comparison to YAML is pretty good; it converts types by default unless you go through extra effort to force something to be a string.

A few points. First, many database drivers don't convert their input to strings, and will raise type errors if you pass an integer when it expects a string (mysql and mysql2 do for the username). Second it's not a difficult change to have the adapters do the conversions. Sequel ships with many more connection adapters than ActiveRecord and it's a nonissue for Sequel. Third, YAML lets you force something to be a string, but this commit doesn't let you do that for connection strings, so the comparison is invalid. Fourth, integer and float conversions don't necessarily round trip ("0100".to_i.to_s).

I'm not an ActiveRecord user, so this doesn't affect me personally. I'm just trying to warn against what I think is an obvious design flaw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 31, 2013
  1. @sodabrew

    DATABASE_URL parsing should turn numeric strings into numeric types, and

    sodabrew authored
    the strings true and false into boolean types, in order to match how
    YAML would parse the same values from database.yml and prevent
    unexpected type errors in the database adapters.
This page is out of date. Refresh to see the latest.
View
12 activerecord/CHANGELOG.md
@@ -1,5 +1,17 @@
## Rails 4.0.0 (unreleased) ##
+* The DATABASE_URL environment variable now converts ints, floats, and
+ the strings true and false to Ruby types. For example, SQLite requires
+ that the timeout value is an integer, and PostgreSQL requires that the
+ prepared_statements option is a boolean. These now work as expected:
+
+ Example:
+
+ DATABASE_URL=sqlite3://localhost/test_db?timeout=500
+ DATABASE_URL=postgresql://localhost/test_db?prepared_statements=false
+
+ *Aaron Stone*
+
* Relation#merge now only overwrites where values on the LHS of the
merge. Consider:
View
19 activerecord/lib/active_record/connection_adapters/connection_specification.rb
@@ -62,6 +62,10 @@ def resolve_hash_connection(spec) # :nodoc:
ConnectionSpecification.new(spec, adapter_method)
end
+ # For DATABASE_URL, accept a limited concept of ints and floats
+ SIMPLE_INT = /^\d+$/
+ SIMPLE_FLOAT = /^\d+\.\d+$/
+
def connection_url_to_hash(url) # :nodoc:
config = URI.parse url
adapter = config.scheme
@@ -77,6 +81,21 @@ def connection_url_to_hash(url) # :nodoc:
spec.map { |key,value| spec[key] = uri_parser.unescape(value) if value.is_a?(String) }
if config.query
options = Hash[config.query.split("&").map{ |pair| pair.split("=") }].symbolize_keys
+ # If anything looks numeric, make it numeric (e.g. pool count, timeout values, etc.)
+ options.map do |key,value|

This could be each, since the return value is not used.

@rafaelfranca Owner

Will fix later. Thanks

@rafaelfranca Owner

Done 2b60a6b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ options[key] = case value
+ when SIMPLE_INT
+ value.to_i
+ when SIMPLE_FLOAT
+ value.to_f
+ when 'true'
+ true
+ when 'false'
+ false
+ else
+ value
+ end
+ end
spec.merge!(options)
end
spec
View
48 activerecord/test/cases/connection_specification/resolver_test.rb
@@ -8,40 +8,66 @@ def resolve(spec)
Resolver.new(spec, {}).spec.config
end
+ def test_url_invalid_adapter
+ assert_raises(LoadError) do
+ resolve 'ridiculous://foo?encoding=utf8'
+ end
+ end
+
+ # The abstract adapter is used simply to bypass the bit of code that
+ # checks that the adapter file can be required in.
+
def test_url_host_no_db
- skip "only if mysql is available" unless current_adapter?(:MysqlAdapter, :Mysql2Adapter)
- spec = resolve 'mysql://foo?encoding=utf8'
+ spec = resolve 'abstract://foo?encoding=utf8'
assert_equal({
- :adapter => "mysql",
+ :adapter => "abstract",
:host => "foo",
:encoding => "utf8" }, spec)
end
def test_url_host_db
- skip "only if mysql is available" unless current_adapter?(:MysqlAdapter, :Mysql2Adapter)
- spec = resolve 'mysql://foo/bar?encoding=utf8'
+ spec = resolve 'abstract://foo/bar?encoding=utf8'
assert_equal({
- :adapter => "mysql",
+ :adapter => "abstract",
:database => "bar",
:host => "foo",
:encoding => "utf8" }, spec)
end
def test_url_port
- skip "only if mysql is available" unless current_adapter?(:MysqlAdapter, :Mysql2Adapter)
- spec = resolve 'mysql://foo:123?encoding=utf8'
+ spec = resolve 'abstract://foo:123?encoding=utf8'
assert_equal({
- :adapter => "mysql",
+ :adapter => "abstract",
:port => 123,
:host => "foo",
:encoding => "utf8" }, spec)
end
+ def test_url_query_numeric
+ spec = resolve 'abstract://foo:123?encoding=utf8&int=500&float=10.9'
+ assert_equal({
+ :adapter => "abstract",
+ :port => 123,
+ :int => 500,
+ :float => 10.9,
+ :host => "foo",
+ :encoding => "utf8" }, spec)
+ end
+
+ def test_url_query_boolean
+ spec = resolve 'abstract://foo:123?true=true&false=false'
+ assert_equal({
+ :adapter => "abstract",
+ :port => 123,
+ :true => true,
+ :false => false,
+ :host => "foo" }, spec)
+ end
+
def test_encoded_password
- skip "only if mysql is available" unless current_adapter?(:MysqlAdapter, :Mysql2Adapter)
password = 'am@z1ng_p@ssw0rd#!'
encoded_password = URI.encode_www_form_component(password)
- spec = resolve "mysql://foo:#{encoded_password}@localhost/bar"
+ spec = resolve "abstract://foo:#{encoded_password}@localhost/bar"
assert_equal password, spec[:password]
end
end
Something went wrong with that request. Please try again.