Regression in conversion of integer values passed for time attributes #53679

benedikt · 2024-11-20T09:52:46Z

When creating a record with a time (date, datetime, ...) column, passing an integer value causes downstream issues. Prior to Rails 8.0.0, the value was changed to nil, but with Rails 8.0.0 that conversion doesn't happen anymore and the integer value gets passed down into the database layer.

It seems like the behavior is implemented in ActiveRecord::AttributeMethods::TimeZoneConversion::TimeZoneConverter and changed with e875b2d. map_avoiding_infinite_recursion was implemented in a way that changed the value to nil when the result of cast returned the same value as before.

Unfortunately, we only caught this issue in production, where apparently some of our users are sending integer values for time columns via our API. We have a default value on this particular column, so it didn't cause any exceptions before.

Admittedly, even the previous behavior could be considered a bug, because I'm not entirely sure it makes sense or was intended to behave that way specifically.

Steps to reproduce

The following script passes on Rails 7.2.2, but fails on Rails 8.0.0

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  # The tests pass on Rails 7.2.2 and older versions
  # gem "rails", '= 7.2.2' 

  gem "rails", '= 8.0.0'

  gem "sqlite3"
end

require "active_record/railtie"
require "minitest/autorun"

# This connection will do for database-independent bug reports.
ENV["DATABASE_URL"] = "sqlite3::memory:"

class TestApp < Rails::Application
  config.load_defaults Rails::VERSION::STRING.to_f
  config.eager_load = false
  config.logger = Logger.new($stdout)
end
Rails.application.initialize!

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
    t.datetime :posted_at
  end
end

class Post < ActiveRecord::Base; end

class BugTest < ActiveSupport::TestCase
  def test_association_stuff
    post = Post.create!(posted_at: 123)

    assert_nil post.reload.posted_at
  end
end

Expected behavior

In previous versions of Rails the integer value was changed to nil.

Admittedly, I'd expect the integer to be treated as a unix timestamp and converted into a proper time object, but I guess that's a different issue.

Actual behavior

The value is not changed anymore. In SQLite, the value is inserted into the database as an interger. In PostgreSQL, this is attempted as well, but the database will reject that query.

System configuration

Rails version: 8.0.0
Ruby version: 3.3.0

The text was updated successfully, but these errors were encountered:

matthewd · 2024-11-20T12:26:00Z

Admittedly, even the previous behavior could be considered a bug, because I'm not entirely sure it makes sense or was intended to behave that way specifically.

Yeah, we did notice this effect, but while it wasn't the exact original focus of the change, I do think it is a bug fix: if someone opened an issue reporting that, uniquely for datetime columns, Active Record was silently ignoring assigned values of unexpected types and storing nil instead, we would treat that as a data loss bug.

Unfortunately, we only caught this issue in production, where apparently some of our users are sending integer values for time columns via our API. We have a default value on this particular column, so it didn't cause any exceptions before.

Yikes, sorry about that! Without knowing the details of what the column represented etc, I feel like I'd be even more concerned about that unknown number of past records that incorrectly got saved with the column default -- but maybe the very fact you hadn't noticed means you were getting lucky there? (e.g. that the faulty supplied values happened to sufficiently correspond to the default, or something.)

benedikt · 2024-11-20T12:34:53Z

Without knowing the details of what the column represented etc, I feel like I'd be even more concerned about that unknown number of past records that incorrectly got saved with the column default -- but maybe the very fact you hadn't noticed means you were getting lucky there? (e.g. that the faulty supplied values happened to sufficiently correspond to the default, or something.)

Yes, this happened to be the case. The value usually represents the current time which also happened to be the default value. Sending integers is also against the spec of our API, so one could argue that defaulting to nil is okay in that particular case.

I wonder how we continue on from here?

benedikt · 2024-11-20T14:25:22Z

Does this boil down to unexpected behavior of ActiveModel::Type::DateTime (and related ones)?

ActiveModel::Type::DateTime.new.serialize(Time.now) # => 2024-11-20 14:19:48 UTC
ActiveModel::Type::DateTime.new.serialize(Time.now.to_i) # => 1732112388
ActiveModel::Type::DateTime.new.serialize(Object.new) # => #<Object:0x0000000124ff80a0>
ActiveModel::Type::DateTime.new.serialize("foo") # => nil
ActiveModel::Type::DateTime.new.serialize(0..10) # => 0..10

Compare this with the behavior of ActiveModel::Type::Integer

ActiveModel::Type::Integer.new.serialize(Time.now) # => 1732112388
ActiveModel::Type::Integer.new.serialize(Time.now.to_i) # => 1732112388
ActiveModel::Type::Integer.new.serialize(Object.new) # => nil
ActiveModel::Type::Integer.new.serialize("foo") # => nil
ActiveModel::Type::Integer.new.serialize(0..10) # => nil

Previously, with the behavior of TimeZoneConverter in Rails < 8.0.0, the difference in behavior was masked by the TimeZoneConverter turning untouched values into nil.

My gut feeling is that ActiveModel::Type::DateTime.new.serialize(Time.now.to_i) should probably just return a Time (or a similar type), and nil for obviously invalid values.

matthewd · 2024-11-20T15:13:35Z

I'm not sure, to be honest... my recollection is that while Integer is nil-prone, there were also other built-in types that tended more toward exceptions.

benedikt · 2024-11-20T16:04:44Z

It seems like the behavior is a bit all over the place.

Behavior of various types

ActiveModel::Type::Value.new.serialize(nil) # => nil
ActiveModel::Type::Value.new.serialize(Time.now) # => 2024-11-20 16:45:32.757355 +0100
ActiveModel::Type::Value.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::Value.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::Value.new.serialize("foo") # => "foo"
ActiveModel::Type::Value.new.serialize(0..10) # => 0..10

ActiveModel::Type::Time.new.serialize(nil) # => nil
ActiveModel::Type::Time.new.serialize(Time.now) # => 2024-11-20 15:45:32.757355 UTC
ActiveModel::Type::Time.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::Time.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::Time.new.serialize("foo") # => nil
ActiveModel::Type::Time.new.serialize(0..10) # => 0..10

ActiveModel::Type::String.new.serialize(nil) # => nil
ActiveModel::Type::String.new.serialize(Time.now) # => 2024-11-20 16:45:32.757355 +0100
ActiveModel::Type::String.new.serialize(Time.now.to_i) # => "1732117532"
ActiveModel::Type::String.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::String.new.serialize("foo") # => "foo"
ActiveModel::Type::String.new.serialize(0..10) # => 0..10

ActiveModel::Type::ImmutableString.new.serialize(nil) # => nil
ActiveModel::Type::ImmutableString.new.serialize(Time.now) # => 2024-11-20 16:45:32.757355 +0100
ActiveModel::Type::ImmutableString.new.serialize(Time.now.to_i) # => "1732117532"
ActiveModel::Type::ImmutableString.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::ImmutableString.new.serialize("foo") # => "foo"
ActiveModel::Type::ImmutableString.new.serialize(0..10) # => 0..10

ActiveModel::Type::BigInteger.new.serialize(nil) # => nil
ActiveModel::Type::BigInteger.new.serialize(Time.now) # => 1732117532
ActiveModel::Type::BigInteger.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::BigInteger.new.serialize(Object.new) # => nil
ActiveModel::Type::BigInteger.new.serialize("foo") # => nil
ActiveModel::Type::BigInteger.new.serialize(0..10) # => nil

ActiveModel::Type::Float.new.serialize(nil) # => nil
ActiveModel::Type::Float.new.serialize(Time.now) # => 1732117532.757355
ActiveModel::Type::Float.new.serialize(Time.now.to_i) # => 1732117532.0
ActiveModel::Type::Float.new.serialize(Object.new) # => NoMethodError: undefined method `to_f' for an instance of Object
ActiveModel::Type::Float.new.serialize("foo") # => 0.0
ActiveModel::Type::Float.new.serialize(0..10) # => NoMethodError: undefined method `to_f' for an instance of Range

ActiveModel::Type::Decimal.new.serialize(nil) # => nil
ActiveModel::Type::Decimal.new.serialize(Time.now) # => 0.2024e4
ActiveModel::Type::Decimal.new.serialize(Time.now.to_i) # => 0.1732117532e10
ActiveModel::Type::Decimal.new.serialize(Object.new) # => 0.0
ActiveModel::Type::Decimal.new.serialize("foo") # => 0.0
ActiveModel::Type::Decimal.new.serialize(0..10) # => 0.0

ActiveModel::Type::DateTime.new.serialize(nil) # => nil
ActiveModel::Type::DateTime.new.serialize(Time.now) # => 2024-11-20 15:45:32.757355 UTC
ActiveModel::Type::DateTime.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::DateTime.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::DateTime.new.serialize("foo") # => nil
ActiveModel::Type::DateTime.new.serialize(0..10) # => 0..10

ActiveModel::Type::Date.new.serialize(nil) # => nil
ActiveModel::Type::Date.new.serialize(Time.now) # => Wed, 20 Nov 2024
ActiveModel::Type::Date.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::Date.new.serialize(Object.new) # => #<Object:0x0000000154973c40>
ActiveModel::Type::Date.new.serialize("foo") # => nil
ActiveModel::Type::Date.new.serialize(0..10) # => 0..10

ActiveModel::Type::Boolean.new.serialize(nil) # => nil
ActiveModel::Type::Boolean.new.serialize(Time.now) # => true
ActiveModel::Type::Boolean.new.serialize(Time.now.to_i) # => true
ActiveModel::Type::Boolean.new.serialize(Object.new) # => true
ActiveModel::Type::Boolean.new.serialize("foo") # => true
ActiveModel::Type::Boolean.new.serialize(0..10) # => true

ActiveModel::Type::Binary.new.serialize(nil) # => nil
ActiveModel::Type::Binary.new.serialize(Time.now) # => #<ActiveModel::Type::Binary::Data:0x000000015495e598 @value="2024-11-20 16:45:32 +0100">
ActiveModel::Type::Binary.new.serialize(Time.now.to_i) # => #<ActiveModel::Type::Binary::Data:0x000000015495e458 @value="1732117532">
ActiveModel::Type::Binary.new.serialize(Object.new) # => #<ActiveModel::Type::Binary::Data:0x000000015495e318 @value="#<Object:0x0000000154973c40>">
ActiveModel::Type::Binary.new.serialize("foo") # => #<ActiveModel::Type::Binary::Data:0x000000015495e200 @value="foo">
ActiveModel::Type::Binary.new.serialize(0..10) # => #<ActiveModel::Type::Binary::Data:0x000000015495e0e8 @value="0..10">

ActiveModel::Type::Integer.new.serialize(nil) # => nil
ActiveModel::Type::Integer.new.serialize(Time.now) # => 1732117532
ActiveModel::Type::Integer.new.serialize(Time.now.to_i) # => 1732117532
ActiveModel::Type::Integer.new.serialize(Object.new) # => nil
ActiveModel::Type::Integer.new.serialize("foo") # => nil
ActiveModel::Type::Integer.new.serialize(0..10) # => nil

Ruby code to generate the values

types = [
  ActiveModel::Type::Value,
  ActiveModel::Type::Time,
  ActiveModel::Type::String,
  ActiveModel::Type::ImmutableString,
  ActiveModel::Type::BigInteger,
  ActiveModel::Type::Float,
  ActiveModel::Type::Decimal,
  ActiveModel::Type::DateTime,
  ActiveModel::Type::Date,
  ActiveModel::Type::Boolean,
  ActiveModel::Type::Binary,
  ActiveModel::Type::Integer
]

values = [
  'nil',
  'Time.now',
  'Time.now.to_i',
  'Object.new',
  '"foo"',
  '0..10'
].to_h { |ruby| [ruby, ruby.instance_eval(ruby)] }

types.each do |type|
  values.each do |ruby, value|
    print "#{type}.new.serialize(#{ruby}) # => "

    result = type.new.serialize(value)

    puts result.inspect
  rescue => e
    puts "#{e.class}: #{e.message}"
  end

  puts
end

To root this discussion a bit more in reality, I'm currently wondering how and where to work around this issue. We're accepting these values via our JSON API. Clients are supposed to submit a valid ISO8601 datetime value, but nothing is stopping them from sending an unix timestamp integer.

Previously, the "invalid" value was ignored, at least allowing the request to succeed and saving all the other data passed in the request. On Rails 8.0.0, this now results in an exception and the entire request fails. The best way would be to fix the clients, but we don't control them.

It feels like the correct place to "fix" this is in the corresponding ActiveModel::Type. I suppose we can monkey-patch it, but also feel like we can't be the only ones running into this?

benedikt · 2024-12-10T08:43:54Z

For anyone else running into this issue, we ended up monkey-patching ActiveModel::Type::DateTime.

module ActiveModel::Type::DateTimeExtensions
  def cast_value(value)
    value = number_to_time(value) if value.is_a?(Numeric)

    super
  end

private

  def number_to_time(value)
    return value if value == Float::INFINITY || value == -Float::INFINITY

    Time.zone.at(value)
  end

  ActiveModel::Type::DateTime.prepend(self)
end

skipkayhil mentioned this issue Jan 3, 2025

ActiveRecord::StatementInvalid: PG::DatetimeFieldOverflow: ERROR: date/time field value out of range #54110

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression in conversion of integer values passed for time attributes #53679

Regression in conversion of integer values passed for time attributes #53679

benedikt commented Nov 20, 2024

matthewd commented Nov 20, 2024

benedikt commented Nov 20, 2024

benedikt commented Nov 20, 2024 •

edited

Loading

matthewd commented Nov 20, 2024

benedikt commented Nov 20, 2024

benedikt commented Dec 10, 2024

Regression in conversion of integer values passed for time attributes #53679

Regression in conversion of integer values passed for time attributes #53679

Comments

benedikt commented Nov 20, 2024

Steps to reproduce

Expected behavior

Actual behavior

System configuration

matthewd commented Nov 20, 2024

benedikt commented Nov 20, 2024

benedikt commented Nov 20, 2024 • edited Loading

matthewd commented Nov 20, 2024

benedikt commented Nov 20, 2024

benedikt commented Dec 10, 2024

benedikt commented Nov 20, 2024 •

edited

Loading