Skip to content

Commit

Permalink
Do not type cast all the database url values.
Browse files Browse the repository at this point in the history
We should only type cast when we need to use.

Related to 4b005fb
  • Loading branch information
rafaelfranca committed Feb 24, 2013
1 parent 5fc3b87 commit e54acf1
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 56 deletions.
6 changes: 3 additions & 3 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,8 @@

*Justin George*

* The `DATABASE_URL` environment variable now converts ints, floats, and
the strings true and false to Ruby types. For example, SQLite requires
* The database adpters now converts the options passed thought `DATABASE_URL`
environment variable to the proper Ruby types before using. 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:

Expand All @@ -161,7 +161,7 @@
DATABASE_URL=sqlite3://localhost/test_db?timeout=500
DATABASE_URL=postgresql://localhost/test_db?prepared_statements=false

*Aaron Stone*
*Aaron Stone + Rafael Mendonça França*

* `Relation#merge` now only overwrites where values on the LHS of the
merge. Consider:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,30 @@ class AbstractAdapter
include MonitorMixin
include ColumnDumper

SIMPLE_INT = /\A\d+\z/

define_callbacks :checkout, :checkin

attr_accessor :visitor, :pool
attr_reader :schema_cache, :last_use, :in_use, :logger
alias :in_use? :in_use

def self.type_cast_config_to_integer(config)
if config =~ SIMPLE_INT
config.to_i
else
config
end
end

def self.type_cast_config_to_boolean(config)
if config == "false"
false
else
config
end
end

def initialize(connection, logger = nil, pool = nil) #:nodoc:
super()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def has_default?
return false if blob_or_text_column? #mysql forbids defaults on blob and text columns
super
end

def blob_or_text_column?
sql_type =~ /blob/i || type == :text
end
Expand Down Expand Up @@ -140,7 +140,7 @@ def initialize(connection, logger, connection_options, config)
@connection_options, @config = connection_options, config
@quoted_column_names, @quoted_table_names = {}, {}

if config.fetch(:prepared_statements) { true }
if self.class.type_cast_config_to_boolean(config.fetch(:prepared_statements) { true })
@visitor = Arel::Visitors::MySQL.new self
else
@visitor = BindSubstitution.new self
Expand Down Expand Up @@ -581,7 +581,7 @@ def limited_update_conditions(where_sql, quoted_table_name, quoted_primary_key)
end

def strict_mode?
@config.fetch(:strict, true)
self.class.type_cast_config_to_boolean(@config.fetch(:strict, true))
end

protected
Expand Down Expand Up @@ -722,7 +722,7 @@ def configure_connection
# Increase timeout so the server doesn't disconnect us.
wait_timeout = @config[:wait_timeout]
wait_timeout = 2147483 unless wait_timeout.is_a?(Fixnum)
variables[:wait_timeout] = wait_timeout
variables[:wait_timeout] = self.class.type_cast_config_to_integer(wait_timeout)

# Make MySQL reject illegal values rather than truncating or blanking them, see
# http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html#sqlmode_strict_all_tables
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,6 @@ 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 = /\A\d+\z/
SIMPLE_FLOAT = /\A\d+\.\d+\z/

def self.connection_url_to_hash(url) # :nodoc:
config = URI.parse url
adapter = config.scheme
Expand All @@ -89,28 +85,11 @@ def self.connection_url_to_hash(url) # :nodoc:
if config.query
options = Hash[config.query.split("&").map{ |pair| pair.split("=") }].symbolize_keys

options.each { |key, value| options[key] = type_cast_value(value) }

spec.merge!(options)
end

spec
end

def self.type_cast_value(value)
case value
when SIMPLE_INT
value.to_i
when SIMPLE_FLOAT
value.to_f
when 'true'
true
when 'false'
false
else
value
end
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ def cache
def initialize(connection, logger, connection_options, config)
super
@statements = StatementPool.new(@connection,
config.fetch(:statement_limit) { 1000 })
self.class.type_cast_config_to_integer(config.fetch(:statement_limit) { 1000 }))
@client_encoding = nil
connect
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ class BindSubstitution < Arel::Visitors::PostgreSQL # :nodoc:
def initialize(connection, logger, connection_parameters, config)
super(connection, logger)

if config.fetch(:prepared_statements) { true }
if self.class.type_cast_config_to_boolean(config.fetch(:prepared_statements) { true })
@visitor = Arel::Visitors::PostgreSQL.new self
else
@visitor = BindSubstitution.new self
Expand All @@ -492,15 +492,15 @@ def initialize(connection, logger, connection_parameters, config)

connect
@statements = StatementPool.new @connection,
config.fetch(:statement_limit) { 1000 }
self.class.type_cast_config_to_integer(config.fetch(:statement_limit) { 1000 })

if postgresql_version < 80200
raise "Your version of PostgreSQL (#{postgresql_version}) is too old, please upgrade!"
end

initialize_type_map
@local_tz = execute('SHOW TIME ZONE', 'SCHEMA').first["TimeZone"]
@use_insert_returning = @config.key?(:insert_returning) ? @config[:insert_returning] : true
@use_insert_returning = @config.key?(:insert_returning) ? self.class.type_cast_config_to_boolean(@config[:insert_returning]) : true
end

# Clears the prepared statements cache.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def sqlite3_connection(config) # :nodoc:
:results_as_hash => true
)

db.busy_timeout(config[:timeout]) if config[:timeout]
db.busy_timeout(ConnectionAdapters::SQLite3Adapter.type_cast_config_to_integer(config[:timeout])) if config[:timeout]

ConnectionAdapters::SQLite3Adapter.new(db, logger, config)
end
Expand Down Expand Up @@ -107,10 +107,10 @@ def initialize(connection, logger, config)

@active = nil
@statements = StatementPool.new(@connection,
config.fetch(:statement_limit) { 1000 })
self.class.type_cast_config_to_integer(config.fetch(:statement_limit) { 1000 }))
@config = config

if config.fetch(:prepared_statements) { true }
if self.class.type_cast_config_to_boolean(config.fetch(:prepared_statements) { true })
@visitor = Arel::Visitors::SQLite.new self
else
@visitor = BindSubstitution.new self
Expand Down
21 changes: 0 additions & 21 deletions activerecord/test/cases/connection_specification/resolver_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,6 @@ def test_url_port
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
password = 'am@z1ng_p@ssw0rd#!'
encoded_password = URI.encode_www_form_component(password)
Expand Down

0 comments on commit e54acf1

Please sign in to comment.