WIP fix for beginning_of_minute removes time zone information in some specific scenarios #43583

[JGShaw]

Summary

WIP on a fix for #43583, provides a failing test for the bug as a start for more development.

[ghiculescu]

Thanks for putting a test up @JGShaw

Do you have any ideas on where to start in terms of a fix?

I just remembered #42583 which feels like it could be related. It may be worth checking if that is the cause; I suspect the fix will be in a similar part of the codebase either way.

[JGShaw]

So I've been taking a look into the code and and found some interesting behaviour. When writing the test I was only able to recreate the issue using Time.parse, and not Time.new

Looks like its something to do with this:

t1 = Time.parse("October 31st 2021 01:00:00 +0100")
t2 = Time.new(2021,10,31,1,0,0, "+0100")
t1 == t2 # -> true
t1.zone # -> "BST"
t2.zone # -> nil

Here is the last section of Time::change:

rails/activesupport/lib/active_support/core_ext/time/calculations.rb

Lines 158 to 168 in 396b22d

	if new_offset
	::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, new_offset)
	elsif utc?
	::Time.utc(new_year, new_month, new_day, new_hour, new_min, new_sec)
	elsif zone&.respond_to?(:utc_to_local)
	::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, zone)
	elsif zone
	::Time.local(new_year, new_month, new_day, new_hour, new_min, new_sec)
	else
	::Time.new(new_year, new_month, new_day, new_hour, new_min, new_sec, utc_offset)
	end

In the t2 case it falls to the elsecase in and preserves the time zone info by just passing utc_offset to Time.new.

In the t1 case it checks zone&.respond_to?(:utc_to_local) which fails as "BST" is a string (at least when I'm running through a debugger?) String but falls into the zone case so calls Time.local with the new year, month, etc. Time.local then is unable to know if it should be 01:00 +00:00 or 01:00 +01:00

Removing thezone&.respond_to?(:utc_to_local) and / or the zone check can make the new test case pass, but makes other tests fail.

[ghiculescu]

Hmmm, this is confusing. It looks like Time#zone can return a String (https://ruby-doc.org/core-3.0.1/Time.html#method-i-zone) but the timezone argument of Time.new doesn't accept a String (https://ruby-doc.org/core-3.0.1/Time.html#class-Time-label-Timezone+argument).

If you get rid of this branch what happens

 elsif zone 
   ::Time.local(new_year, new_month, new_day, new_hour, new_min, new_sec) 

[JGShaw]

Removing that branch makes the new test pass along with all of the other direct change tests. However it does make lots of tests fail that cross a daylight savings time boundary using methods like Time#beginning_of_day, which is why I'm guessing Time.local was used.

Failure:
TimeExtCalculationsTest#test_beginning_of_day [/vagrant/rails/activesupport/test/core_ext/time_ext_test.rb:154]:
start DST.
Expected: 2006-04-02 00:00:00 -0500
  Actual: 2006-04-02 00:00:00 -0400

[ghiculescu]

Suggested change

	with_env_tz "GB" do
	with_env_tz "BST" do

for the record, this makes the test pass.

[ghiculescu]

I've tried a bunch of other things but couldn't find any other way to get it passing. Two questions

1. Why GB as the time zone? Where did that come from?
2. I think in general you should use Time.zone.parse for this sort of thing - any reason you're not here?

[JGShaw]

1. I believe GB is what should be there, based on others not being timezones, US/Eastern and NZ etc. It might pass with BST as its not a valid region, so defaults to another region which does leave daylight savings time at the same time as the GB region, but thats just me guessing. All the with_env_tz "GB" does is sets ENV['TZ'] = "GB", so I think it has to be one of these codes: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

2. We only noticed the issue during the hour that the clocks went back in the UK, in the production code we were using we had Time.now.beginning_of_minute, but given Time#beginning_of_minute just calls change(sec: 0) the problem is not actually with Time#beginning_of_minute. Using Time.zone was the only way to recreate it in a test. I had managed to also recreate it using Timecop but I was not going to introduce that here.

[ghiculescu]

The reason I'm suggesting maybe it should be "BST" is because that's what Time.parse(...).zone is returning. But I'm not really sure.

FWIW in our production apps we don't set ENV["TZ"] at all, and we use https://www.rubydoc.info/gems/rubocop/0.57.2/RuboCop/Cop/Rails/TimeZone to remind people to use Time.zone.now... instead of Time.now....

All of which is to say, I'm not really sure if this is a bug or not just yet.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.