Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveSupport::TimeWithZone gets converted to DateTime during coercion #53913

Open
radiantshaw opened this issue Dec 11, 2024 · 2 comments
Open

Comments

@radiantshaw
Copy link

radiantshaw commented Dec 11, 2024

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

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

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails"
  gem "rspec"
end

require "active_support"
require "active_support/core_ext/time"

class CustomDateTime
  def <=>(other)
    0
  end

  def coerce(other)
    [self, other]
  end
end

RSpec.describe "BugTest" do
  let(:date_time) do
    ActiveSupport::TimeWithZone.new(Time.new, ActiveSupport::TimeZone["America/Los_Angeles"])
  end
  let(:custom_date_time) { CustomDateTime.new }

  it "converts ActiveSupport::TimeWithZone to DateTime" do
    expect(custom_date_time).to receive(:<=>).and_wrap_original do |original_method, other|
      expect(other).to be_an(ActiveSupport::TimeWithZone) # ! ! ! THIS SHOULD PASS ! ! !
      original_method.call(other)
    end

    date_time <=> custom_date_time
  end
end

Expected behavior

The spec should pass because the other argument passed to <=> should be the same object that was sent the <=> message, instead it is an instance of DateTime.

  • Since DateTime doesn't have the zone information, it ends up returning wrong values when calling strftime on it.
  • It also makes it impossible to create value objects that can be compared with an instance of ActiveSupport::TimeWithZone

Actual behavior

The <=> method of the CustomDateTime class receives an instance of DateTime.

System configuration

Rails version: 7.0.8.5

Ruby version: 3.2.4

@pixeltrix
Copy link
Contributor

A number of points:

  1. Your coerce method should return self as the second argument, e.g.

    def coerce(other)
       [<coercion-of-other-value-here>, self]
    end
  2. Given the incorrect order in yours, the <=> method is called on the instance of AS::TWZ. This calls <=> on the instance of Time and that triggers this block of code:

    def compare_with_coercion(other)
    # we're avoiding Time#to_datetime and Time#to_time because they're expensive
    if other.class == Time
    compare_without_coercion(other)
    elsif other.is_a?(Time)
    # also avoid ActiveSupport::TimeWithZone#to_time before Rails 8.0
    if other.respond_to?(:comparable_time)
    compare_without_coercion(other.comparable_time)
    else
    compare_without_coercion(other.to_time)
    end
    else
    to_datetime <=> other
    end
    end

    The other in this block is your instance of CustomDateTime and since it is neither a Time or Time subclass it hits the else block that converts the AS::TWZ to a DateTime. This DateTime instance receives <=> which calls coerce on your CustomDateTime and the order flips back and your CustomDateTime receives the <=> and returns 0.

  3. Your spec setup seems wrong - I couldn't drop a binding.irb in the block. This version seems to work as you expect it to:

    expect(custom_date_time).to receive(:<=>).and_wrap_original do |original_method, other|
      expect(other).not_to be_an(ActiveSupport::TimeWithZone)
      original_method.call(other)
    end

    As you can see from the explanation above the <=> gets called on CustomDateTime after the conversion of the original value to DateTime which is why the spec passes.

  4. The Active Support date/time methods don't support coerce in general because numeric operations beyond a simple <=> don't make sense, e.g. this works as expected:

    Time.current + 2.days

    But this fails with a TypeError:

    2.days + Time.current

    However, numeric operations with ActiveSupport::Duration do make sense and I suggest checking out that code to get a clearer understanding of how coerce works.

@radiantshaw
Copy link
Author

Thanks for the reply @pixeltrix. Didn't get a chance to look into this last week. Here's what I noticed about your suggestions.

  1. Swapping the elements in the array (as you suggested) throws the program into an infinite loop. Which is correct because coerce is supposed to return an Array where the first element is the one on whom you expect the <=> to be forwarded to. In my case, that'd be self since I'm trying to tell Ruby that I'll handle the coercion instead of letting ActiveSupport::TimeWithZone do it for me.
  2. This is helpful in knowing how things work. Thank you so much for pointing me to this bit of code.
  3. Yeah you're right. I've updated the specs to what you suggested. Also inverted the expect condition so that the expected and actual behaviour are better understood. Sorry for the confusion earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants