MONGOID-5490: Deprecate :use_activesupport_time_zone

[johnnyshields]

I'll raise a follow-up PR for Mongoid 9.0.

[johnnyshields]

Ready for review

[sandstrom]

Is there a way to continue using Time instances?

We have this across 85 database fields, and changing to ActiveSupport::TimeWithZone would be quite a lot of work throughout our app (affects type checking, etc).

For example, would a workaround be for us to defined Time as our own custom field type? Would that take precedence over the default ext (https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/time.rb)?

https://www.mongodb.com/docs/mongoid/master/reference/fields/#custom-field-types

Our app is only using UTC internally and is never concerned with timezones (for us that's a client concern), so we'd rather not be forced to use TimeWithZone.

---

Also, I must say I don't really see the benefit that this change brings.

Instead of allowing storage of one of Ruby's built-in objects, it's no longer going to be possible to store vanilla Time in the database.

And the change itself seems to be just this:

# BEFORE
if use_activesupport_time_zone == false
  return time
else
  return time.in_time_zone(…)
end

# AFTER
return time

In other words, the maintenance burden of keeping this option shouldn't be huge.

[jamis]

Hello @sandstrom,

Please forgive me if I'm misunderstanding your concern, but are you worried that the following will no longer be supported?

class MyModel
  include Mongoid::Document
  field :timestamp, type: Time
end

m = MyModel.create(timestamp: Time.now)

Using Time as a field type is still supported, even after this change. Have you experienced something breaking as a result of the PR? If so, please let us know; it's probably a bug.

[sandstrom]

@jamis Thanks for chiming in! 😄

Our problem is that:

MyModel.create(timestamp: Time.now).timestamp.class #=> 'ActiveSupport::TimeWithZone
# instead of returning an instance of `Time`

This causes problems with typing (e.g. Sorbet) and type checks such as m.timestamp.class === 'Time' now being false.

Although TimeWithZone is using some trickery to pretend being an instance of Time, it de-facto isn't.

https://github.com/rails/rails/blob/a11f0a63673d274c59c69c2688c63ba303b86193/activesupport/lib/active_support/time_with_zone.rb#L499-L501

---

So my question regarding this change would be, how important is this deprecation, and why is it needed?

Supporting transparent (in/out) support for Ruby's built-in types into the database, i.e. let me just pass in my Time and get it back unchanged, seems like a pretty basic feature. One that Mongoid also has had for a long time! So why remove that?

I see how some might feel that timezones should be handled by the database, but I'd argue that it's quite common to do what we are doing, and just operating at UTC everywhere internally in our system (timezone handling, which we do a lot btw, are a client-side concern for us).

[jamis]

@sandstrom -- what version of Mongoid are you using? It seems to work okay for me with Mongoid 9:

class MyModel
  include Mongoid::Document

  field :timestamp, type: Time
end

m = MyModel.create(timestamp: Time.now)
p m.timestamp.class #=> Time
p m.reload.timestamp
p m.timestamp.class #=> Time

[sandstrom]

@jamis We've downgraded to 8.0 for now, because this was causing problems in our code base.

Hmm, strange. I cannot really square that with the information in the deprecation message, this PR and the source code.

This is what the code on master does:

https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/time.rb#L59

This is 9.0:

https://github.com/mongodb/mongoid/blob/v9.0.2/lib/mongoid/extensions/time.rb#L59

[johnnyshields]

@sandstrom

The changes in Mongoid 9.0 affect how time values are deserialized from the database, specifically it now returns ActiveSupport::TimeWithZone (AS::TWZ) in 100% of cases, and removes the previously deprecated settings to allow returning Time. As @jamis mentions, this happens (intentionally) even when you specify field type: Time. Aside from affecting Sorbet checks, given that AS::TWZ is fully compatible with Time I would not expect it to affect your app behavior itself.

Returning AS::TWZ is the correct default for the vast majority of Rails apps that use Mongoid--this is also what ActiveRecord does. This article provides some background; if you need to use timezones at all (most apps do) Time is unwieldy.

@jamis -- I do think makes sense to re-introduce a non-default option that allows returning Time (not AS::TWZ) for apps that are strictly UTC-only, maybe call it use_native_time. Strictly speaking, Time has ~3x better CPU performance than AS::TWZ (but both are very fast, so this is a negligible difference in most real-world apps)

[sandstrom]

The changes in Mongoid 9.0 affect how time values are deserialized from the database, specifically it now returns ActiveSupport::TimeWithZone (AS::TWZ) in 100% of cases, and removes the previously deprecated settings to allow returning Time. As @jamis mentions, this happens (intentionally) even when you specify field type: Time. Aside from affecting Sorbet checks, given that AS::TWZ is fully compatible with Time I would not expect it to affect your app behavior itself.

Thanks, yes I know how it works, I looked at the source code.

Returning AS::TWZ is the correct default for the vast majority of Rails apps that use Mongoid--this is also what ActiveRecord does. This article provides some background; if you need to use timezones at all (most apps do) Time is unwieldy.

I've read the article and I see the point for server-generated HTML. But in our case the app is an API that treats everything as UTC, and timezones is a client-side concern (Javascript layer). This works really well for single-page apps and mobile apps, where we have access to the browser/phone local timezone and can 'convert at the edge'.

But for us, I'd argue that using vanilla Time works great and comes with zero downsides. We support all the IANA timezones in our app with this approach, and have had no problems with it (again, since it's treated as a front-end concern).

I do think makes sense to re-introduce a non-default option that allows returning Time (not AS::TWZ) for apps that are strictly UTC-only, maybe call it use_native_time.

This would be awesome and would totally solve our use-case (and I'm guessing we're not the only ones)! 😄 🎉

[jamis]

We've downgraded to 8.0 for now, because this was causing problems in our code base.

@sandstrom -- I'm really sorry that this has been causing you grief. I'd like to understand better exactly how this is problematic; can you share an example or two of the kind of problem you're running into? As @johnnyshields said, ActiveSupport::TimeWithZone is supposed to be API-compatible with Time, and the translation to-and-from in Mongoid ought to be transparent. I feel like any exceptions to that are probably bugs that we should address.

I'm generally uncomfortable with adding more knobs and dials to Mongoid, as they usually make it harder to understand behavior. We can look into adding that config option back, but as I said, I really want to make sure I understand (and can replicate) the issue being reported here, first.

[sandstrom]

@jamis Same thing as I wrote above basically. Even though it has the same methods, it's still a different class.

This breaks type checking in a bunch of places for us.

Below are a few simple code examples on how to see that they are different classes:

t1 = Time.now #=> 2024-10-08 08:13:04.232558 +0000
t2 = t1.in_time_zone('UTC') # => Tue, 08 Oct 2024 08:13:04.232558000 UTC +00:00
t1.class #=> Time
t2.class #=> ActiveSupport::TimeWithZone

# different output
t1.to_s #=> "2024-10-08 08:13:04 +0000"
t2.to_s #=> "2024-10-08 08:13:04 UTC"

# different classes
t1.class == t2.class #=> false

[jamis]

@sandstrom -- the bit that I'm struggling with is that when I query the class of a Time-valued field, I get Time, not ActiveSupport::TimeWithZone. I really need a concrete example that I can use to evaluate the impact here.

Please let me know:

• Your OS
• Your Ruby version
• Your Rails version
• Your mongoid gem version
• Your mongo gem version
• The version of MongoDB that you're querying
• A minimal script that I can run locally that exhibits the issue
• Any other relevant environmental information that might help me debug this

I know that, for you, the problem is obvious, and painful, and I'm sorry. Please help it to be obvious and painful to me, as well, so that I can understand what is fundamentally going on here.

[sandstrom]

@jamis Happy to help, but the way I'm reading the code it's configured to return an instance of ActiveSupport::TimeWithZone? Isn't that what this PR was all about?

https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/time.rb#L59

@johnnyshields What you said above doesn't see to square with the experience that @jamis has, i.e. he doesn't get an instance of ActiveSupport::TimeWithZone back.

The changes in Mongoid 9.0 affect how time values are deserialized from the database, specifically it now returns ActiveSupport::TimeWithZone (AS::TWZ) in 100% of cases, and removes the previously deprecated settings to allow returning Time.

But maybe I've misunderstood something and you are talking about different things?

Details

• macOS Sonoma 14.5 (23F79)
• ruby 3.2.4 (2024-04-23 revision af471c0e01) [arm64-darwin23]
• Rails 7.1.4
• mongoid 8.1.6 (now), but we had 9.0.1 when this was a problem (we've reverted for now)
• mongo gem 2.20.1
• mongodb 7.0.12
• [script to reproduce; I can get back to you on this one if needed]

[jamis]

@sandstrom Let's try it this way, then. Here's a script that, when I run it, prints the following:

$ ruby 5490.rb
Ruby: 3.2.4
Mongo: 2.20.1
Mongoid: 9.0.1
Rails: 7.1.4
after creation, `t.time` is a Time
after reload, `t.time` is a Time

Can you run it and let me know what it tells you?

The script itself:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'mongo', '=2.20.1'
  gem 'mongoid', '=9.0.1'
  gem 'rails', '=7.1.4'
end

puts "Ruby: #{RUBY_VERSION}"
puts "Mongo: #{Mongo::VERSION}"
puts "Mongoid: #{Mongoid::VERSION}"
puts "Rails: #{Rails.version}"

Mongoid.connect_to '5490-sandbox'

class Troubleshooting
  include Mongoid::Document

  field :time, type: Time
end

t = Troubleshooting.create!(time: Time.now)

puts "after creation, `t.time` is a #{t.time.class.inspect}"

t = Troubleshooting.find(t.id)
puts "after reload, `t.time` is a #{t.time.class.inspect}"

[jamis]

Ah, I think I understand the issue now. Mongoid.use_utc defaults to false, which will cause Mongoid.time_zone to return whatever time zone is configured on Time.zone (nil, by default). When that is nil, Time#in_time_zone returns a plain Time instance.

I suspect you've got Mongoid.use_utc = true, though, which uses "UTC" as the time zone. With this set, I also get ActiveSupport::TimeWithZone instances.

Does that sound like how you've got things configured on your end?

[sandstrom]

@jamis Correct!

Time.zone #=> #<ActiveSupport::TimeZone:0x00000001051a8758 @name="UTC", @tzinfo=#<TZInfo::DataTimezone: Etc/UTC>, @utc_offset=nil>

Mongoid.use_utc #=> true

[jamis]

I'm still not clear on exactly how the ActiveSupport::TimeWithZone instances are causing problems. Pretend I know nothing about Sorbet, and have never used any type checkers with Ruby before. Can you give me something I could run locally, that would help me feel the pain you're experiencing there?

[sandstrom]

@jamis Thanks for asking!

In addition to type checking here are two examples of code that would break:

## EXAMPLE 1

# This is a simplified example of type checking that we have in a few places,
# in our system. For example when interfacing with external APIs, that only accept
# some types of data, we need to first verify that our upstream code doesn't send anything
# that the API cannot handle.
my_object = {
  'created_at': Time.now,
  'updated_at': Time.now,
  'name': 'John Doe',
  'email': '[email protected]',
  'age': 30,
  '…': '…',
}

# make sure we only send scalar values
my_object.each do |key, value|
  permitted_scalars = [String, Integer, Float, TrueClass, FalseClass, NilClass, Time, Date]

  unless permitted_scalars.include?(value.class)
    my_object.delete(key) # this broke when we updated to Mongoid 9
  end
end

# continue with the rest of the code

## EXAMPLE 2

# Here is an example of an internal serializer class expecting a time
# class. It will raise if it's passed a value that isn't Time.
class UserSerializer < Base
  type 'User'
  description 'An employee at your company'

  field :name, type: String, description: 'Full name'
  field :email, type: String, description: 'Primary email'
  field :company_name, type: String, description: 'Company name'
  field :previous_sign_ins, type: Array, subtype: Time
  field :last_sign_in, type: Time, description: 'Last sign in time'

  association :home_location, nullable: true, blueprint: LocationSerializer
  association :work_location, nullable: true, blueprint: LocationSerializer
end

In short, all our data is stored in the database and will pass through Mongoid, and 100% of our objects have one or more time fields.

We have ~100 models and a code base of ~800k lines of code. Swapping out one class for another is bound to cause a lot of trouble.

One could argue that we should move the entire code base over to using ActiveRecord::TimeWithZone instead. But that's a pretty daunting task, with tens of thousands of lines of code that we would have to update.

[jamis]

Thanks @sandstrom -- I appreciate the additional information. That does help.

As you said in your last paragraph:

One could argue that we should move the entire code base over to using ActiveRecord::TimeWithZone instead. But that's a pretty daunting task, with tens of thousands of lines of code that we would have to update.

I think this is spot on, on both counts. In an ideal world, you'd be able to update your type checking to at least include TimeWithZone, or to accommodate duck typing (object.acts_like?(:time)). With a legacy code base, this is not always a luxury we can afford, though.

I am still reluctant to restore the :use_activesupport_time_zone flag; it's a legacy feature, and though it's not a complicated one, it is one more fiddly bit that we have to support and remember.

However, it should be possible for you to work around this by monkey patching Mongoid::Config.time_zone:

module PatchTimeZone
  def time_zone
    nil
  end
end

Mongoid::Config.extend(PatchTimeZone)

The Mongoid::Config.time_zone method is only used in that one place in Mongoid, and so changing it to return nil shouldn't have any other side effects (unless you happen to be calling it yourself, or possibly using a library that does). It's worth trying, at least, and seeing how it works for you.

[sandstrom]

@jamis Your decision since you are the maintainer! 😄

I understand why you want to deprecate and remove the existing flag. To signal that people should avoid using it. That makes sense to me!

However, if I were you I'd also consider what Johnny suggested above (#5488 (comment)), adding use_native_time or a similar configuration value, that would default to false.

You'd provide a migration path for others, and supporting it would be trivial since it's just an if/else block.

if config.use_native_time == true
  return time
else
  return time.in_time_zone(…)
end

[sandstrom]

@jamis Do you want me to provide a PR in line with my suggestion in the comment above?

[jamis]

Hey @sandstrom, I'm reluctant to add another dial here, especially since it would be primarily a crutch for legacy code. There would be little or no benefit to new projects that are just now adopting Mongoid, which means the interface adds (just a little) more mental overhead for new adopters.

It's easy to justify adding something like this because it adds "just a little" overhead, but I hope you can see that it's the precedent that concerns me here. We justify adding this one, which makes it easier to justify adding another one somewhere else, and so forth. At some point, we need to draw a line in the sand and say "this is how it is." And sadly, whenever a line is drawn, there will always be someone on the other side of it who wishes it had been drawn "just a little" bit closer to them. I really do hate saying "no," but I think it needs to be said, here.

I'd hate for you to put in the work on something that I honestly don't feel has much hope of being merged. In your case, a shim (the monkey patch I proposed) will hopefully help your existing code work with the newer behavior.

Things that would make me reconsider this include:

• Discovering a reasonably common use case that would cause new code (as opposed to legacy code) to be impacted by the current behavior.
• Learning that a significant number of users have stumbled over this. (As it is, you're the first person we've heard from who has tripped over this in the 6 months since Mongoid 9 was released.)

I hope you know that we have heard you, and we understand that this isn't an ideal solution for you. I really am sorry about that. :/

[sandstrom]

@jamis I understand, it's a reasonable argument!

Have a great weekend! 😄