Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago
Summary: Koha::Calendar->is_holiday date truncation creates fatal errors for TZ Americ...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 16.05
Hardware: All All
: P5 - low critical (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 13079 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-04-28 06:31 UTC by David Cook
Modified: 2017-01-29 11:38 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago (1.44 KB, patch)
2016-05-23 02:40 UTC, David Cook
Details | Diff | Splinter Review
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago (1.80 KB, patch)
2016-05-30 01:24 UTC, David Cook
Details | Diff | Splinter Review
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago (1.86 KB, patch)
2016-06-03 07:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16376: (regression tests) (2.63 KB, patch)
2016-12-06 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago (2.55 KB, patch)
2016-12-06 14:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2016-04-28 06:31:45 UTC
Koha::Calendar->is_holiday calls the following DateTime truncate method:

$localdt->truncate( to => 'day' );

This will truncate 2015-09-06 12:00:00 down to 2015-09-06 00:00:00. 

This causes major problems in Santiago, Chile, because 2015-09-06 is an invalid local time due to DST. 

I can't explain why it's a problem in 2015 because DST is weird in Chile in 2015, but it's obviously a problem for every other year as well: http://www.timeanddate.com/time/change/chile/santiago?year=2015

Consider 2014:
2014	Sunday, 27 April, 12:00 Midnight	Sunday, 7 September, 12:00 Midnight

You shouldn't have a problem on 27 April, because when 27 April 12:00 midnight hits (relative to UTC), local time is changed to 27 April 11:00pm. While UTC marches on, local time has been magically moved back an hour. 

You will have a problem on 7 September 12:00 midnight though, because that time doesn't exist. As soon as we roll over from 6 September 11:59pm to the next minute... we're actually on to 7 September 01:00:00am. Midnight doesn't exist on that day, which creates the following error in DateTime.pm:

"Invalid local time for date in time zone: America/Santiago"

This is a problem that the DateTime writer predicted. You can see their comments here:
http://cpansearch.perl.org/src/DROLSKY/DateTime-1.26/lib/DateTime.pm
https://rt.cpan.org/Public/Bug/Display.html?id=93347

We should probably do something like:

eval {
    $dt->truncate(to => 'day');
} || warn "Eval failed with '$@'\n";

Why are we even doing this day truncation?
Comment 1 David Cook 2016-04-28 06:32:07 UTC
You can easily verify this with the following Perl code:

use DateTime;

my $dt = DateTime->new(
    month => 9,
    year => 2015,
    day => 6,
    hour => 12,
    minute => 00,
    second => 00,
    time_zone => 'America/Santiago',
);
print "DateTime:\n";
print $dt."\n";
print "Timezone:\n";
print $dt->time_zone()->name."\n";
print "Let's truncate...\n";
$dt->truncate( to => 'day');


TZ=America/Santiago perl issues.pl

DateTime:
2015-09-06T12:00:00
Timezone:
America/Santiago
Let's truncate...
Invalid local time for date in time zone: America/Santiago
Comment 2 David Cook 2016-04-28 06:35:14 UTC
This problem was brought to me by Larry Letelier who found that he couldn't use overdue_notices.pl because of this error.

You can replicate it by creating a Koha::Calendar, and then running the following:

use DateTime;
use Koha::DateUtils;
use Koha::Calendar;


my $dt = DateTime->new(
    month => 9,
    year => 2015,
    day => 6,
    hour => 12,
    minute => 00,
    second => 00,
    time_zone => 'America/Santiago',
);
my $now = DateTime->now();
my $calendar = Koha::Calendar->new(branchcode => "MAIN");
$calendar->days_between($dt, $now);

Run that using the America/Santiago TZ environmental variable:
TZ=America/Santiago perl issues.pl

--

You should get the "Invalid local time for date in time zone: America/Santiago" message and the script should die.
Comment 3 David Cook 2016-04-28 06:49:11 UTC
Ahh, I bet it's truncating it to compare it to "single_holidays" and "exception_holidays"... right... that makes sense.

In that case, before truncating it, I think you have to turn it into a "floating" DateTime. 

Except that we're using the Timezone in single_holidays() and exception_holidays()... that's great...

I think we might need to re-think Koha::Calendar in light of https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12669
Comment 4 Katrin Fischer 2016-04-29 00:26:54 UTC
*** Bug 13079 has been marked as a duplicate of this bug. ***
Comment 5 Tomás Cohen Arazi 2016-05-17 14:40:15 UTC
(In reply to David Cook from comment #3)
> Ahh, I bet it's truncating it to compare it to "single_holidays" and
> "exception_holidays"... right... that makes sense.
> 
> In that case, before truncating it, I think you have to turn it into a
> "floating" DateTime. 
> 
> Except that we're using the Timezone in single_holidays() and
> exception_holidays()... that's great...
> 
> I think we might need to re-think Koha::Calendar in light of
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12669

Hey, where's the patch?
Comment 6 David Cook 2016-05-17 23:54:49 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> Hey, where's the patch?

I've been too busy working on other projects :/. 

While I might have time to write a patch, I don't have anywhere near the time to test it or write a comprehensive test plan for others to use... :/.
Comment 7 Tomás Cohen Arazi 2016-05-18 00:27:25 UTC
(In reply to David Cook from comment #6)
> (In reply to Tomás Cohen Arazi from comment #5)
> > Hey, where's the patch?
> 
> I've been too busy working on other projects :/. 
> 
> While I might have time to write a patch, I don't have anywhere near the
> time to test it or write a comprehensive test plan for others to use... :/.

If you write a patch, I write regression tests for it :-)
Comment 8 David Cook 2016-05-23 01:56:46 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> If you write a patch, I write regression tests for it :-)

Thanks, Tomas :).

Used 'grep -l -R "Koha::Calendar" * --exclude-dir="blib" | xargs grep "exception_holidays"' to try to find everything using Koha::Calendar and exception_holidays... looks like it's only Koha::Calendar.

Looks like it's only Koha::Calendar::exeption_holidays() and Koha::Calendar::is_holiday() as well.

I'm sure this issue pervades Koha, but perhaps we can resolve the problem in this particular piece of it...
Comment 9 David Cook 2016-05-23 02:40:16 UTC Comment hidden (obsolete)
Comment 10 M. Tompsett 2016-05-26 02:19:17 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> If you write a patch, I write regression tests for it :-)

There's a patch. Where are the regression tests, Tomás? ;)
Comment 11 Jonathan Druart 2016-05-26 09:36:28 UTC
(In reply to David Cook from comment #9)
> Using a floating zone allows us to use the same day, month, year
> for comparison purposes without running into the possibility of
> creating an invalid local time and thus a fatal software error.

Ok you will remove the software error, but I am not sure it's the way to go.

Currently the dates are stored in local time (so with the tz), if we want to switch to floating time zones (what we should do at some point!) I think we should recalculate all dates present in DB.
An easy workaround would be to use Koha::DateUtils, which deals (at least try to) with invalid dates due du DST.
Comment 12 Jonathan Druart 2016-05-26 09:39:12 UTC
Moreover `grep time_zone Koha/Calendar.pm` will return another occurrence where the fix should be applied too.
Comment 13 David Cook 2016-05-30 01:07:16 UTC
(In reply to Jonathan Druart from comment #11)
> Ok you will remove the software error, but I am not sure it's the way to go.
> 

I would certainly prefer a more thorough overhaul of Koha's DateTime handling, but I think this is good enough for now. Chile can't use the overdue functionality in Koha at all at this stage without this fix.

> Currently the dates are stored in local time (so with the tz), if we want to
> switch to floating time zones (what we should do at some point!) I think we
> should recalculate all dates present in DB.
> An easy workaround would be to use Koha::DateUtils, which deals (at least
> try to) with invalid dates due du DST.

Well, we calculate and interpret the dates using local time, but they're stored without timezone in the database.

So I'm not sure what you mean by "recalculate all dates present in DB". The only difference between "local" and "floating" is the timezone. They're the same numbers, so there would be no recalculation to do. Or do you mean we should recalculate into UTC?

Even if it were stored in UTC, it would need to be converted to local, and then to floating for the actual date handling, as we'd need the local numbers before doing any datetime math/truncation for the holiday checks.

I have my reservations about Koha::DateUtils... I don't think enough thought has been put into its implementation. Besides, it wouldn't work in this case.

Jonathan, the main problem at the moment is that Koha::DateUtils produces a valid local time, so it uses the local timezone. During the $calendar->days_between(), a day is added to this local datetime to produce another valid local time. However, the act of truncating that valid local time creates an invalid local time. So Koha::DateUtils isn't going to know there's a problem until it's too late. You could implement your own wrapper around DateTime, but I think that's getting excessive. We just need to be thoughtful and careful in our handling of dates and time.
Comment 14 David Cook 2016-05-30 01:18:57 UTC
(In reply to Jonathan Druart from comment #12)
> Moreover `grep time_zone Koha/Calendar.pm` will return another occurrence
> where the fix should be applied too.

Interesting.

I think I avoided that the first time because $calendar->is_holiday() uses the following:

my $ymd   = $localdt->ymd('')  ;
if ($self->single_holidays(  $ymd  ) == 1 ) {
    return 1;
}

However, you are correct. sub single_holidays is weird... but definitely prone to this problem too.
Comment 15 David Cook 2016-05-30 01:24:44 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2016-06-03 07:42:19 UTC
Created attachment 51968 [details] [review]
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago

Using a DateTime object with a timezone of America/Santiago
was causing fatal errors for Koha::Calendar->is_holiday
and Koha::Calendar->exception_holidays, when the objects
were truncated to an invalid local time.

Using a floating zone allows us to use the same day, month, year
for comparison purposes without running into the possibility of
creating an invalid local time and thus a fatal software error.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 17 Jonathan Druart 2016-06-11 15:05:33 UTC
(In reply to David Cook from comment #13)
> (In reply to Jonathan Druart from comment #11)
> > Ok you will remove the software error, but I am not sure it's the way to go.
> > 
> 
> I would certainly prefer a more thorough overhaul of Koha's DateTime
> handling, but I think this is good enough for now. Chile can't use the
> overdue functionality in Koha at all at this stage without this fix.
> 
> > Currently the dates are stored in local time (so with the tz), if we want to
> > switch to floating time zones (what we should do at some point!) I think we
> > should recalculate all dates present in DB.
> > An easy workaround would be to use Koha::DateUtils, which deals (at least
> > try to) with invalid dates due du DST.
> 
> Well, we calculate and interpret the dates using local time, but they're
> stored without timezone in the database.
> 
> So I'm not sure what you mean by "recalculate all dates present in DB". The
> only difference between "local" and "floating" is the timezone. They're the
> same numbers, so there would be no recalculation to do. Or do you mean we
> should recalculate into UTC?

If we plan to support several timezone, I think we will need to recalculate the existing dates into UTC, yes.

> Even if it were stored in UTC, it would need to be converted to local, and
> then to floating for the actual date handling, as we'd need the local
> numbers before doing any datetime math/truncation for the holiday checks.

Yes, floating times for calculation and tz to display/store the values. But let's have this discussion somewhere else :)

> I have my reservations about Koha::DateUtils... I don't think enough thought
> has been put into its implementation. Besides, it wouldn't work in this case.

Well, look at the code before and after, we you will understand how it has been useful :)

Looking at your patch again, it looks good.
Please provide tests to highlight the issue you are trying to fix and to make sure this patch won't introduce regressions.
Comment 18 OpenGeek 2016-06-13 14:22:09 UTC
​This patch solve the repodted issue with overdue_process.pl using TZ=America/Santiago 

In fact, overdue_process.pl calculated until 1985 without errors.
(Do not ask me why they have issues with that date :P )


Many Thanks David, Chris, John, and all involved !!
Comment 19 Tomás Cohen Arazi 2016-06-13 14:26:27 UTC
(In reply to OpenGeek from comment #18)
> ​This patch solve the repodted issue with overdue_process.pl using
> TZ=America/Santiago 
> 
> In fact, overdue_process.pl calculated until 1985 without errors.
> (Do not ask me why they have issues with that date :P )
> 
> 
> Many Thanks David, Chris, John, and all involved !!

Hey, this one is failed-qa, I promised a regression test which I haven't provided and Jonathan failed it!
Comment 20 Kyle M Hall 2016-09-08 17:53:56 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> (In reply to OpenGeek from comment #18)
> > ​This patch solve the repodted issue with overdue_process.pl using
> > TZ=America/Santiago 
> > 
> > In fact, overdue_process.pl calculated until 1985 without errors.
> > (Do not ask me why they have issues with that date :P )
> > 
> > 
> > Many Thanks David, Chris, John, and all involved !!
> 
> Hey, this one is failed-qa, I promised a regression test which I haven't
> provided and Jonathan failed it!

Ping! Any chance we can get those unit tests ; )
Comment 21 Tomás Cohen Arazi 2016-12-05 19:00:17 UTC
I spent a couple hours on this. And now have to leave for Manuel's birthday :-D So, for the record:

- I narrowed the problem down to the sub days_between. Which is called in the failing scripts. The only way to reproduce this (in order to create the unit tests) was:

    $ENV{TZ} = $timezone;
    use POSIX qw(tzset);
    tzset;
    my $tz = C4::Context->tz;

    my $chile_dt = DateTime->new(
        day       => 6,
        month     => 9,
        year      => 2015,
        time_zone => $tz
    );
    my $now_dt = DateTime->now;

    my $branch = $builder->build({ source => 'Branch' })->{branchcode};
    my $calendar = Koha::Calendar->new( branchcode => $branch );
    my $diff = $calendar->days_between($now_dt,$chile_dt);

Using the other suspected functions didn't trigger the problem (exception_holidays, etc)[1]. So the problem was correctly highlighted by David, but I'm not sure what's the best place to fix it. I vote for days_between.

I'll try to resume tomorrow, and will be glad to hear people's comments, specially David, that created the patch.

[1] Using truncate didn't raise the problem to me, either.
Comment 22 Tomás Cohen Arazi 2016-12-06 14:40:14 UTC
Created attachment 58001 [details] [review]
Bug 16376: (regression tests)

This patch introduces a regression test for exception_holidays. This routine
returns a list of datetimes to be used in date comparison and some datetimes don't exist
in some timezones, so floating timezones should be used instead.

To test:
- Apply the patch on master
- Run:
  $ prove t/db_dependent/Holidays.t
=> FAIL: The new test fails

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2016-12-06 14:40:23 UTC
Created attachment 58002 [details] [review]
Bug 16376 - Koha::Calendar->is_holiday date truncation creates fatal errors for TZ America/Santiago

Using a DateTime object with a timezone of America/Santiago
was causing fatal errors for Koha::Calendar->is_holiday
and Koha::Calendar->exception_holidays, when the objects
were truncated to an invalid local time.

Using a floating zone allows us to use the same day, month, year
for comparison purposes without running into the possibility of
creating an invalid local time and thus a fatal software error.

Edit:

While the changes to is_holiday and single_holiday make sense (Jonathan agrees too)
I didn't manage to have them fail, because truncate is not failing in my trials, but
days_between. So to me, it narrows down to have exception_holiday return floating tz
datetime objects so it doesn't break days_between.

Anyway, it is ok to push this patch, and the regression test I provide covers this scenario
I'm describing.

To test:
- Apply the regression tests patch
- Run:
  $ prove t/db_dependent/Holidays.t
=> FAIL: Unexpected error due to bad timezone/date combination
- Apply this patch
- Run:
  $ prove t/db_dependent/Holidays.t
=> SUCCESS: Tests pass

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Kyle M Hall 2016-12-09 15:25:54 UTC
Pushed to master for 17.05, thanks David, Tomas!
Comment 25 Katrin Fischer 2016-12-12 22:08:31 UTC
These patches have been pushed to 16.11.x, will be in 16.11.01.
Comment 26 Julian Maurice 2017-01-02 10:51:13 UTC
Pushed to 3.22.x for 3.22.14
Comment 27 David Cook 2017-01-12 05:49:02 UTC
Hurray! Take that invalid local time for date in time zone...
Comment 28 Mason James 2017-01-29 11:38:58 UTC
Pushed to 16.05.x, for 16.05.06 release