Ugh, this is a bit of a mess - unintended consequences spread across six years 😢

So the reason that ActiveRecord::Type::Time::Value was introduced was to handle a bug in MariaDB where it wouldn’t ignore the date part of a timestamp when comparing against a TIME column unlike other databases. This is a class that isn’t documented and shouldn’t be relied upon to exist - therefore we should definitely not dump it out as YAML.

I think the fix to restore previous behaviour should be to add the following:

::YAML.dump_tags[ActiveRecord::Type::Time::Value] = "!ruby/object:ActiveSupport::TimeWithZone"
::YAML.load_tags['!ruby/object:ActiveRecord::Type::Time::Value'] = 'ActiveSupport::TimeWithZone'

We can put these in Active Record so that we don’t tie Active Support to it. The load_tags change will handle any YAML that’s been dumped since the change in db9ba18 was made and convert them to AS::TWZ instances which is what would happen with Rails 6.1.

There’s a wider issue of what TIME columns should be dumped as - YAML does support them, and they get loaded as seconds since midnight, e.g.

>> YAML.load("lunch: 12:00")
=> {"lunch"=>43200}

However that’s outside the scope of this issue.

I’m going to have a closer look at the papertrail failure - I’d like to know how these instances are being exposed.

I agree with @pedrocarmona but slightly amended. Add to the bottom of activerecord/lib/active_record/type/time.rb

::YAML.dump_tags[ActiveRecord::Type::Time::Value] = ::YAML.dump_tags[ActiveSupport::TimeWithZone]

It looks like the intent has always been that AR Time::Value delegates yaml serialization to TimeWithZone. TimeWithZone changed how it implements yaml serialization because of the way Psych works:

https://github.com/ruby/psych/blob/e8d262fa108a1605443d7426c185452b538775be/lib/psych/visitors/yaml_tree.rb#L498

Psych calls Class.name before calling encode_with. Because Class.name triggers the new deprecation warning the coding / serialization behavior was changed to rely on dump_tags But dump tags is sensitive to the actual class being serialized so needs to be aware of any classes that might rely on TimeWithZone to perform serialization (because the old behavior allowed TimeWithZone to set the YAML tag

I did a grep for any other classes that used DelegateClass(::Time) and didn’t find any, so I think this is the only file that needs to change.

@pixeltrix can you weigh in here?

So the ActiveModel::Type::Value#serialize docs says this:

Casts a value from the ruby type to a type that the database knows how to understand.
The returned value from this method should be a String, Numeric, Date, Time, Symbol, true, false, or nil.

However this isn’t true for Active Record types - there’s a whole host of internal, non-documented classes that can get returned for non-standard column types for different database, e.g. OID::Array::Data for PostgreSQL adapter. The docs also aren’t true for certain Active Model types either - e.g. Binary returns a custom class as well (not sure why it just doesn’t return a binary-encoded string).

Most of these types get serialised in PaperTrail using the default mechanism, e.g. this is an int array in PostgreSQL:

>> puts YAML.dump p.type_for_attribute("tags").serialize([1,2])
--- !ruby/struct:ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Array::Data
encoder: !ruby/object:PG::TextEncoder::Array
  name: integer[]
  elements_type:
values:
- 1
- 2

That’s because they’re mostly PORO classes, however the time one is special which makes me think that the problem lies in that class. I’m going to investigate what impacts changing that to a PORO class has on the rest of the tests. This would all be so much easier if Ruby had a time of day class 😅

Do we still need to account for things that have been generated since 7.0 release?

Hopefully the deserialize method I suggested would handle that - the Value objects are unwound and passed to the super method so all non-Value objects should behave as before.

It is also a change (maybe better though?) that you used to get a TWZ directly back from a ::YAML#load but now you will get back an AR::Time::Value if someone’s got some overly specific is_a? kind of logic they might get broken?

You only get an ActiveRecord::Type::Time::Value when calling serialize in preparation for using in a database query so apps are very unlikely to be affected, therefore the main source of issues is likely to be gems like papertrail that hook into that layer somehow. I can look into some other gems to see if there might be any issues - any suggestions like alternative connection adaptors? After MySQL, PostgreSQL, SQLite, Oracle and SQL Server are there any other adaptors worth looking at?

The changes specifically apply to the behavior of Time::Value when obj_of_ClassToDelegateTo is aTimeWithZone

https://apidock.com/ruby/Object/DelegateClass/instance

class MyClass < DelegateClass(ClassToDelegateTo) # Step 1
  def initialize
    super(obj_of_ClassToDelegateTo)              # Step 2
  end

I ran a git bisect (shakes fist mimemagic and curses) using this script:

require "bundler"
Bundler.setup

require "rails/all"
require "active_support/all"

Time.zone = "America/Chicago"
output = YAML.dump(ActiveRecord::Type::Time::Value.new(Time.current))
puts output
if output.starts_with?("--- !ruby/object:ActiveRecord::Type::Time::Value")
	puts "BAD"
	exit(false)
elsif output.starts_with?("--- !ruby/object:ActiveSupport::TimeWithZone")
	puts "GOOD"
	exit(true)
else
	puts "Indeterminate"
	exit(false)
end

It is definitely commit db9ba1865916985212b02a73e24500555532be2b that introduces the breaking change

Because Time::Value delegates it’s YAML serialization behavior to it’s obj_of_ClassToDelegateTo

When obj_of_ClassToDelegateTo is a TimeWithZone that means that YAML#dump will eventually send encode_with to Time::Value which delegates to obj_of_ClassToDelegateTo which is TimeWithZone

Before commit db9ba1865916985212b02a73e24500555532be2b TimeWithZone#encode_with would explicitly set the tag of the coder And it would do this no matter the underlying type of the class being serialized.

After commit db9ba1865916985212b02a73e24500555532be2b the tag is set via the built-in behavior of Psych by looking in dump_tags

But because dump_tags is a simple Hash with the key being the type it only gets the correct tag when the type is specifically TimeWithZone

In order to maintain compatibility with previous behavior any class that delegates YAML serialization behavior to TimeWithZone via encode_with needs to output the same tag as TimeWithZone

@Intrepidd have you checked what @pedrocarmona suggested in the rail 7 PR?

Yes but this is way below my pay grade unfortunately, I don’t have the context about the changes that were done around ActiveRecord::Type::Time::Value and YAML dumping

@pixeltrix Maybe we should add

::YAML.dump_tags[ActiveRecord::Type::Time::Value] = "!ruby/object:ActiveSupport::TimeWithZone"

Wdyt?