Skip to content

Commit

Permalink
Refactor microsecond precision to be database agnostic
Browse files Browse the repository at this point in the history
The various databases don't actually need significantly different
handling for this behavior, and they can achieve it without knowing
about the type of the object.

The old implementation was returning a string, which will cause problems
such as breaking TZ aware attributes, and making it impossible for the
adapters to supply their logic for time objects.
  • Loading branch information
sgrif committed Feb 10, 2015
1 parent f926c1c commit f1a0fa9
Show file tree
Hide file tree
Showing 11 changed files with 55 additions and 84 deletions.
Expand Up @@ -127,7 +127,12 @@ def quoted_date(value)
end
end

value.to_s(:db)
result = value.to_s(:db)
if value.respond_to?(:usec) && value.usec > 0
"#{result}.#{sprintf("%06d", value.usec)}"

This comment has been minimized.

Copy link
@dre-hh

dre-hh Mar 5, 2015

We cannot allways append microseconds. it will ignore the Time::DATE_FORMATS[:db] setting, which can be set cusomly, e.g
Time::DATE_FORMATS[:db] = "%Y-%m-%d %H:%M:%S.%3N"
See my comment to df5a38f

I see there is a precision option for dates now. Let's just stick to it and remove the #{sprintf("%06d", value.usec)}"

Rails seems to set a default for Time::DATE_FORMATS[:db] to "%Y-%m-%d %H:%M:%S"

We have to either change the default to "%Y-%m-%d %H:%M:%S.%6N" for mysql > 5.6 and other db's that support it or just stick to the default without microseconds allowing the user to reconfigure it, either by Time::DATE_FORMATS[:db] or with the precision option within a migraiton

See issue #19223

This comment has been minimized.

Copy link
@sgrif

sgrif Mar 5, 2015

Author Contributor

Time::DATE_FORMATS[:db] isn't a "setting", and it should not be overridden

else
result
end
end

def prepare_binds_for_database(binds) # :nodoc:
Expand Down
Expand Up @@ -29,7 +29,7 @@ def prepare_column_options(column)

limit = column.limit || native_database_types[column.type][:limit]
spec[:limit] = limit.inspect if limit
spec[:precision] = column.precision.inspect if column.precision
spec[:precision] = column.precision.inspect if column.precision && column.precision != 0
spec[:scale] = column.scale.inspect if column.scale

default = schema_default(column) if column.has_default?
Expand Down
Expand Up @@ -263,18 +263,6 @@ def index_algorithms
{}
end

# QUOTING ==================================================

# Quote date/time values for use in SQL input. Includes microseconds
# if the value is a Time responding to usec.
def quoted_date(value) #:nodoc:
if value.acts_like?(:time) && value.respond_to?(:usec)
"#{super}.#{sprintf("%06d", value.usec)}"
else
super
end
end

# Returns a bind substitution value given a bind +column+
# NOTE: The column param is currently being used by the sqlserver-adapter
def substitute_at(column, _unused = 0)
Expand Down Expand Up @@ -409,15 +397,15 @@ def column_name_for_operation(operation, node) # :nodoc:
protected

def initialize_type_map(m) # :nodoc:
register_class_with_limit m, %r(boolean)i, Type::Boolean
register_class_with_limit m, %r(char)i, Type::String
register_class_with_limit m, %r(binary)i, Type::Binary
register_class_with_limit m, %r(text)i, Type::Text
register_class_with_limit m, %r(date)i, Type::Date
register_class_with_limit m, %r(time)i, Type::Time
register_class_with_limit m, %r(datetime)i, Type::DateTime
register_class_with_limit m, %r(float)i, Type::Float
register_class_with_limit m, %r(int)i, Type::Integer
register_class_with_limit m, %r(boolean)i, Type::Boolean
register_class_with_limit m, %r(char)i, Type::String
register_class_with_limit m, %r(binary)i, Type::Binary
register_class_with_limit m, %r(text)i, Type::Text
register_class_with_precision m, %r(date)i, Type::Date
register_class_with_precision m, %r(time)i, Type::Time
register_class_with_precision m, %r(datetime)i, Type::DateTime
register_class_with_limit m, %r(float)i, Type::Float
register_class_with_limit m, %r(int)i, Type::Integer

m.alias_type %r(blob)i, 'binary'
m.alias_type %r(clob)i, 'text'
Expand Down Expand Up @@ -451,6 +439,13 @@ def register_class_with_limit(mapping, key, klass) # :nodoc:
end
end

def register_class_with_precision(mapping, key, klass) # :nodoc:
mapping.register_type(key) do |*args|
precision = extract_precision(args.last)
klass.new(precision: precision)
end
end

def extract_scale(sql_type) # :nodoc:
case sql_type
when /\((\d+)\)/ then 0
Expand Down
Expand Up @@ -712,11 +712,6 @@ def initialize_type_map(m) # :nodoc:
m.alias_type %r(year)i, 'integer'
m.alias_type %r(bit)i, 'binary'

m.register_type(%r(datetime)i) do |sql_type|
precision = extract_precision(sql_type)
MysqlDateTime.new(precision: precision)
end

m.register_type(%r(enum)i) do |sql_type|
limit = sql_type[/^enum\((.+)\)/i, 1]
.split(',').map{|enum| enum.strip.length - 2}.max
Expand All @@ -734,6 +729,14 @@ def register_integer_type(mapping, key, options) # :nodoc:
end
end

def extract_precision(sql_type)
if /datetime/ === sql_type
super || 0
else
super
end
end

def fetch_type_metadata(sql_type, collation = "", extra = "")
MysqlTypeMetadata.new(super(sql_type), collation: collation, extra: extra, strict: strict_mode?)
end
Expand Down Expand Up @@ -916,14 +919,6 @@ def create_table_definition(name, temporary = false, options = nil, as = nil) #
TableDefinition.new(native_database_types, name, temporary, options, as)
end

class MysqlDateTime < Type::DateTime # :nodoc:
private

def has_precision?
precision || 0
end
end

class MysqlString < Type::String # :nodoc:
def type_cast_for_database(value)
case value
Expand All @@ -945,7 +940,7 @@ def cast_value(value)
end

def type_classes_with_standard_constructor
super.merge(string: MysqlString, date_time: MysqlDateTime)
super.merge(string: MysqlString)
end
end
end
Expand Down
Expand Up @@ -5,15 +5,6 @@ module OID # :nodoc:
class DateTime < Type::DateTime # :nodoc:
include Infinity

def type_cast_for_database(value)
if has_precision? && value.acts_like?(:time) && value.year <= 0
bce_year = format("%04d", -value.year + 1)
super.sub(/^-?\d+/, bce_year) + " BC"
else
super
end
end

def cast_value(value)
if value.is_a?(::String)
case value
Expand Down
25 changes: 11 additions & 14 deletions activerecord/lib/active_record/type/date_time.rb
Expand Up @@ -11,28 +11,25 @@ def type
end

def type_cast_for_database(value)
return super unless value.acts_like?(:time)

zone_conversion_method = ActiveRecord::Base.default_timezone == :utc ? :getutc : :getlocal

if value.respond_to?(zone_conversion_method)
value = value.send(zone_conversion_method)
if precision && value.respond_to?(:usec)
number_of_insignificant_digits = 6 - precision
round_power = 10 ** number_of_insignificant_digits
value = value.change(usec: value.usec / round_power * round_power)
end

return value unless has_precision?
if value.acts_like?(:time)
zone_conversion_method = ActiveRecord::Base.default_timezone == :utc ? :getutc : :getlocal

result = value.to_s(:db)
if value.respond_to?(:usec) && (1..6).cover?(precision)
"#{result}.#{sprintf("%0#{precision}d", value.usec / 10 ** (6 - precision))}"
else
result
if value.respond_to?(zone_conversion_method)
value = value.send(zone_conversion_method)
end
end

value
end

private

alias has_precision? precision

def cast_value(string)
return string unless string.is_a?(::String)
return if string.empty?
Expand Down
7 changes: 0 additions & 7 deletions activerecord/test/cases/adapters/mysql2/connection_test.rb
Expand Up @@ -122,11 +122,4 @@ def test_logs_name_rename_column_sql
ensure
@connection.execute "DROP TABLE `bar_baz`"
end

if mysql_56?
def test_quote_time_usec
assert_equal "'1970-01-01 00:00:00.000000'", @connection.quote(Time.at(0))
assert_equal "'1970-01-01 00:00:00.000000'", @connection.quote(Time.at(0).to_datetime)
end
end
end
11 changes: 4 additions & 7 deletions activerecord/test/cases/adapters/mysql2/datetime_test.rb
Expand Up @@ -4,15 +4,12 @@
class DateTimeTest < ActiveRecord::TestCase
self.use_transactional_fixtures = false

class Foo < ActiveRecord::Base; end

def test_default_datetime_precision
ActiveRecord::Base.connection.create_table(:foos, force: true)
ActiveRecord::Base.connection.add_column :foos, :created_at, :datetime
ActiveRecord::Base.connection.add_column :foos, :updated_at, :datetime
assert_nil activerecord_column_option('foos', 'created_at', 'precision')
teardown do
ActiveRecord::Base.connection.drop_table(:foos, if_exists: true)
end

class Foo < ActiveRecord::Base; end

def test_datetime_data_type_with_precision
ActiveRecord::Base.connection.create_table(:foos, force: true)
ActiveRecord::Base.connection.add_column :foos, :created_at, :datetime, precision: 1
Expand Down
5 changes: 0 additions & 5 deletions activerecord/test/cases/adapters/postgresql/quoting_test.rb
Expand Up @@ -27,11 +27,6 @@ def test_quote_float_infinity
assert_equal "'Infinity'", @conn.quote(infinity)
end

def test_quote_time_usec
assert_equal "'1970-01-01 00:00:00.000000'", @conn.quote(Time.at(0))
assert_equal "'1970-01-01 00:00:00.000000'", @conn.quote(Time.at(0).to_datetime)
end

def test_quote_range
range = "1,2]'; SELECT * FROM users; --".."a"
type = OID::Range.new(Type::Integer.new, :int8range)
Expand Down
3 changes: 3 additions & 0 deletions activerecord/test/cases/adapters/postgresql/timestamp_test.rb
Expand Up @@ -145,10 +145,13 @@ def test_formatting_timestamp_according_to_precision
date = ::Time.utc(2014, 8, 17, 12, 30, 0, 999999)
Foo.create!(created_at: date, updated_at: date)
assert foo = Foo.find_by(created_at: date)
assert_equal 1, Foo.where(updated_at: date).count
assert_equal date.to_s, foo.created_at.to_s
assert_equal date.to_s, foo.updated_at.to_s
assert_equal 000000, foo.created_at.usec
assert_equal 999900, foo.updated_at.usec
ensure
ActiveRecord::Base.connection.drop_table(:foos, if_exists: true)
end

private
Expand Down
10 changes: 5 additions & 5 deletions activerecord/test/cases/quoting_test.rb
Expand Up @@ -46,28 +46,28 @@ def test_quoted_date

def test_quoted_time_utc
with_timezone_config default: :utc do
t = Time.now
t = Time.now.change(usec: 0)
assert_equal t.getutc.to_s(:db), @quoter.quoted_date(t)
end
end

def test_quoted_time_local
with_timezone_config default: :local do
t = Time.now
t = Time.now.change(usec: 0)
assert_equal t.getlocal.to_s(:db), @quoter.quoted_date(t)
end
end

def test_quoted_time_crazy
with_timezone_config default: :asdfasdf do
t = Time.now
t = Time.now.change(usec: 0)
assert_equal t.getlocal.to_s(:db), @quoter.quoted_date(t)
end
end

def test_quoted_datetime_utc
with_timezone_config default: :utc do
t = DateTime.now
t = Time.now.change(usec: 0).to_datetime
assert_equal t.getutc.to_s(:db), @quoter.quoted_date(t)
end
end
Expand All @@ -76,7 +76,7 @@ def test_quoted_datetime_utc
# DateTime doesn't define getlocal, so make sure it does nothing
def test_quoted_datetime_local
with_timezone_config default: :local do
t = DateTime.now
t = Time.now.change(usec: 0).to_datetime
assert_equal t.to_s(:db), @quoter.quoted_date(t)
end
end
Expand Down

0 comments on commit f1a0fa9

Please sign in to comment.