Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit 4b005fb371c2e7af80df7da63be94509b1db038c 1 parent ee4a2bb
@sodabrew sodabrew authored
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|
+ 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

4 comments on commit 4b005fb

@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.

@rafaelfranca

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

Thank you @jeremyevans

@sodabrew

@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.

@jeremyevans

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.

Please sign in to comment.
Something went wrong with that request. Please try again.