Bug 24850 - Koha::DateUtils ignores offsets in RFC3339 datetimes
Summary: Koha::DateUtils ignores offsets in RFC3339 datetimes
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords: rel_20_11_target
Depends on: 24643
Blocks: 24608
  Show dependency treegraph
 
Reported: 2020-03-11 15:23 UTC by Martin Renvoize
Modified: 2020-07-17 09:11 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.35 KB, patch)
2020-03-11 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-11 17:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when rfc3339 format is passed (1.46 KB, patch)
2020-03-11 17:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-13 12:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.07 KB, patch)
2020-03-17 10:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.85 KB, patch)
2020-03-17 14:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.38 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (1.99 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.33 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.71 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.85 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.71 KB, patch)
2020-03-26 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:43 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:44 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct tests for offset handling in RFC3339 dates (1.43 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Correct offset handling in dt_from_string (2.04 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Add tests for invalid RFC3339 dates (1.38 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Don't fallback when date_format is passed (1.76 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Always return 'instance' local time (1.90 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 24850: Continue to handle default time and daylight saving (1.77 KB, patch)
2020-03-26 13:45 UTC, Agustín Moyano
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-03-11 15:23:15 UTC
The final test added in bug 24643 incorrectly tests RFC3339 timezone handling.

If an offset is passed it should be applied to the given time to return an epoch relative to UTC and then the koha timezone config value should be applied against that to give us a koha instance relative time.
Comment 1 Martin Renvoize 2020-03-11 16:22:31 UTC
Created attachment 100585 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 2 Martin Renvoize 2020-03-11 16:22:34 UTC
Created attachment 100586 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 3 Martin Renvoize 2020-03-11 16:22:36 UTC
Created attachment 100587 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 4 Martin Renvoize 2020-03-11 17:56:52 UTC
Created attachment 100592 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 5 Martin Renvoize 2020-03-11 17:56:55 UTC
Created attachment 100593 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 6 Martin Renvoize 2020-03-11 17:56:57 UTC
Created attachment 100594 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 7 Martin Renvoize 2020-03-11 17:57:00 UTC
Created attachment 100595 [details] [review]
Bug 24850: Don't fallback when rfc3339 format is passed
Comment 8 Martin Renvoize 2020-03-11 17:58:49 UTC
I'm not sure about the final followup.. it scares me that we're always falling back to a relatively loose regex at the moment; even if a date_format is passed in.
Comment 9 Martin Renvoize 2020-03-12 10:02:18 UTC
Finally.. I think we also need to add some handling into the DateTime::Format::MySQL datetime_parser that we are using via DBIx::Class.. it does not take into account offsets/timezones in the format_datetime method.
Comment 10 Martin Renvoize 2020-03-13 12:03:11 UTC
Created attachment 100678 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 11 Martin Renvoize 2020-03-13 12:03:14 UTC
Created attachment 100679 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 12 Martin Renvoize 2020-03-13 12:03:17 UTC
Created attachment 100680 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 13 Martin Renvoize 2020-03-13 12:03:20 UTC
Created attachment 100681 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 14 Martin Renvoize 2020-03-13 12:04:27 UTC
(In reply to Martin Renvoize from comment #8)
> I'm not sure about the final followup.. it scares me that we're always
> falling back to a relatively loose regex at the moment; even if a
> date_format is passed in.

Corrected in final followup.
Comment 15 Martin Renvoize 2020-03-17 10:23:43 UTC
Created attachment 100881 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 16 Martin Renvoize 2020-03-17 14:52:27 UTC
Created attachment 100901 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 17 Martin Renvoize 2020-03-17 14:52:30 UTC
Created attachment 100902 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 18 Martin Renvoize 2020-03-17 14:52:34 UTC
Created attachment 100903 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 19 Martin Renvoize 2020-03-17 14:52:37 UTC
Created attachment 100904 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 20 Martin Renvoize 2020-03-17 14:52:40 UTC
Created attachment 100905 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 21 Bernardo Gonzalez Kriegel 2020-03-23 20:30:04 UTC
Martin,
t/DateUtils.t fails always. 
An special setup is needed?
Comment 22 Martin Renvoize 2020-03-26 10:27:59 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #21)
> Martin,
> t/DateUtils.t fails always. 
> An special setup is needed?

Nope.. seems I forgot to check the tests again after than final patch.. looking at it now.
Comment 23 Martin Renvoize 2020-03-26 11:14:20 UTC
Created attachment 101803 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff
Comment 24 Martin Renvoize 2020-03-26 11:14:23 UTC
Created attachment 101804 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.
Comment 25 Martin Renvoize 2020-03-26 11:14:27 UTC
Created attachment 101805 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.
Comment 26 Martin Renvoize 2020-03-26 11:14:30 UTC
Created attachment 101806 [details] [review]
Bug 24850: Don't fallback when date_format is passed
Comment 27 Martin Renvoize 2020-03-26 11:14:33 UTC
Created attachment 101807 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..
Comment 28 Martin Renvoize 2020-03-26 11:14:36 UTC
Created attachment 101808 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time
Comment 29 Agustín Moyano 2020-03-26 13:43:00 UTC
Created attachment 101837 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 30 Agustín Moyano 2020-03-26 13:44:12 UTC
Created attachment 101838 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 31 Agustín Moyano 2020-03-26 13:45:17 UTC
Created attachment 101839 [details] [review]
Bug 24850: Correct tests for offset handling in RFC3339 dates

The tests were incorrectly passing for RFC3339 dates passed with an
offset.  This patch corrects the test.

Test plan:
1/ Read the change
2/ Agree the change adheres to the RFC
3/ Run the test and varify it now fails
3/ Signoff

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 32 Agustín Moyano 2020-03-26 13:45:21 UTC
Created attachment 101840 [details] [review]
Bug 24850: Correct offset handling in dt_from_string

This patch adds correct handling for when an offset is passed within an
RFC3339 formatted datetime.

Test plan
1/ Run the DateUtils test and varify it now passes.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 33 Agustín Moyano 2020-03-26 13:45:25 UTC
Created attachment 101841 [details] [review]
Bug 24850: Add tests for invalid RFC3339 dates

We should die on invalidly formatted dates being passed.  This patch
adds such a test case.

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 34 Agustín Moyano 2020-03-26 13:45:29 UTC
Created attachment 101842 [details] [review]
Bug 24850: Don't fallback when date_format is passed

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 35 Agustín Moyano 2020-03-26 13:45:32 UTC
Created attachment 101843 [details] [review]
Bug 24850: Always return 'instance' local time

This patch adds handling to ensure we always convert a passed in time to
the instance configured timezone..

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 36 Agustín Moyano 2020-03-26 13:45:36 UTC
Created attachment 101844 [details] [review]
Bug 24850: Continue to handle default time and daylight saving

This patch prevents the call to set_time_zone if we are handling a
dateonly datetime string or a DST time

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 37 Jonathan Druart 2020-04-08 10:58:53 UTC
+        # Default to UTC (when 'Z' is passed) for inbound timezone.

Do you have an example where this "Z" is passed?
Comment 38 Martin Renvoize 2020-04-15 07:53:10 UTC
(In reply to Jonathan Druart from comment #37)
> +        # Default to UTC (when 'Z' is passed) for inbound timezone.
> 
> Do you have an example where this "Z" is passed?

Z is part of the RFC ;).  I don't yet have an example of where we pass it in Koha, but it's valid via the API and so should be catered for.
Comment 39 Martin Renvoize 2020-04-15 07:56:17 UTC
https://github.com/mojolicious/json-validator/blob/master/lib/JSON/Validator/Formats.pm#L35 shows we support passing Z in our validation routines.

https://tools.ietf.org/html/rfc3339#section-2 for reference in the RFC
Comment 40 Marcel de Rooy 2020-04-17 07:26:36 UTC
Part of the rfc3339 regex: ([Tt\s])
I am not sure if lowercase t is part of the syntax. I think it should be capital T or space. Note that \s is more than space. I think we should test for space only here.

        # Default to UTC (when 'Z' is passed) for inbound timezone.
        $tz = DateTime::TimeZone->new( name => 'UTC' );

Yes, Z stands for UTC. But this line comes out of the blue. We are just building the regex here.
Further down, we are testing for the offset and at that code location we should check for the Z too and set UTC timezone.

+    # Convert to configured timezone (unless we started with a dateonly string or had to drop to floating time)
+    $dt->set_time_zone($server_tz) unless ( $date_only || $floating );

This line is confusing too. Although I am not sure if we have calls in Koha where a timezone is passed already, so it may be theoretical. But aren't we effectively ignoring/overriding the $tz parameter here?
Should this routine have a tz parameter anyhow?
Note also that we did not interfere earlier in the code: 
    return DateTime->now( time_zone => $tz )
In line with what we do at the end, we should not allow this return?
So, actually the big question is here: Shouldnt we always return in server timezone ?

Changing status.
Comment 41 Martin Renvoize 2020-04-23 08:02:51 UTC
The idea of the input tz is that you're passing in the TZ of the string being passed in.. the idea of the tz conversion at the bottom (and in the call to now) is that we always pass back a DateTime object with a consistent config based timezone.  doing this call at the bottom means we will convert internally to the correct instance timezone from the passed timezone.
Comment 42 Martin Renvoize 2020-07-15 12:41:00 UTC
Did my reply make sense Marcel.. or are you still unhappy with this work.
Comment 43 Marcel de Rooy 2020-07-15 12:42:38 UTC
(In reply to Martin Renvoize from comment #42)
> Did my reply make sense Marcel.. or are you still unhappy with this work.

I am afraid that it got lost between lots of other patch mails..
Keeping this one for Friday..
Comment 44 Martin Renvoize 2020-07-15 19:32:37 UTC
No worries... I forgot about it too.. might take a bit to get the brain back up to speed.
Comment 45 Marcel de Rooy 2020-07-17 09:09:20 UTC
Please rebase
Comment 46 Marcel de Rooy 2020-07-17 09:11:43 UTC
(In reply to Martin Renvoize from comment #41)
> The idea of the input tz is that you're passing in the TZ of the string
> being passed in.. the idea of the tz conversion at the bottom (and in the
> call to now) is that we always pass back a DateTime object with a consistent
> config based timezone.  doing this call at the bottom means we will convert
> internally to the correct instance timezone from the passed timezone.

Ok. But I have the impression that many questions raised in comment40 are not answered yet..