XML-RPC WebService should take and return dates and times in UTC

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
WebService
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Callek)

Tracking

3.5.3
Bugzilla 3.6
Bug Flags:
approval +
approval3.6 +
blocking3.6 +

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
Now that we've got the JSON-RPC WebService returning ISO-8601 dates and times in the UTC timezone, we should make the XML-RPC WebService consistent with it, and return and accept times in UTC instead of in Bugzilla's local timezone.

To keep backward-compatibility with clients, we should modify Bugzilla.time and Bugzilla.timezone to return info in UTC instead of in Bugzilla's local timezone. (To understand why, see the XML-RPC docs that describe the dateTime.iso8601 format.)
Flags: blocking3.6+
(Reporter)

Comment 1

9 years ago
Oh, I'm a liar--apparently the current WebService docs don't explain that all times have to be in Bugzilla's local timezone. I'm surprised; that's kind of a huge oversight.
(Reporter)

Comment 2

9 years ago
(In any case, the docs will have to be modified to explain that as of Bugzilla 3.6, dates and times are always in UTC instead of the server's local timezone.)
(Reporter)

Updated

8 years ago
Keywords: relnote
(Assignee)

Updated

8 years ago
Assignee: webservice → bugspam.Callek
(Reporter)

Comment 3

8 years ago
Added to the release notes in bug 547466.
Keywords: relnote
(Assignee)

Comment 4

8 years ago
Created attachment 428366 [details] [diff] [review]
v1 -- Without POD

Untested, no POD updates and I'm not certain on the offset hard coding for Bugzilla::Time stuff.
Attachment #428366 - Flags: review?(mkanat)
(Reporter)

Comment 5

8 years ago
Comment on attachment 428366 [details] [diff] [review]
v1 -- Without POD

This is the JSON-RPC patch.
Attachment #428366 - Flags: review?(mkanat) → review-
(Assignee)

Comment 6

8 years ago
Created attachment 428379 [details] [diff] [review]
v1 -- Without POD (correct patch)

bah; typo in my diff command generated patch to wrong directory
Attachment #428366 - Attachment is obsolete: true
Attachment #428379 - Flags: review?(mkanat)
(Reporter)

Comment 7

8 years ago
Comment on attachment 428379 [details] [diff] [review]
v1 -- Without POD (correct patch)

>-    $offset = sprintf('%+05d', $offset);
>-    return { timezone => $self->type('string', $offset) };
>+    # All Webservices return times in UTC; Use UTC here for backwards compat.
>+    return { timezone => $self->type('string', "0") };

  Actually, I think that needs to be -0000. Maybe +0000. (I'm not sure what the standard is there for UTC.)

> sub time {
> [snip]
>     return {
>         db_time       => $self->type('dateTime', $db_time),

  DB_TIME also needs to be modified into UTC, by using datetime_from.

>+        tz_offset     => $self->type('string', '0'),

  You'll want to check the current format on this, it's either identical to the format I suggested above or slightly longer.

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>-    # YUI expects ISO8601 in UTC time; uncluding TZ specifier
>+    # YUI expects ISO8601 in UTC time; including TZ specifier

  What's the change here? I can't figure it out.

>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'
>-        # We leave off the $ from the end of this regex to allow for possible
>-        # extensions to the XML-RPC date standard.
>-        $value =~ /^(\d{4})(\d{2})(\d{2})T(\d{2}):(\d{2}):(\d{2})/;
>-        $value = "$1-$2-$3 $4:$5:$6";
>+        # We only support UTC TimeZones inbound
>+        # pass 'Z' specifier to datetime_from to force it
>+        my $converted = datetime_from($value . 'Z', Bugzilla->local_timezone);

  My concern here is that somebody might specify a timezone, even though the XML-RPC spec doesn't allow it, and then appending Z would just mess up the parsing. (This is along the lines of "be liberal in what you accept".)
Attachment #428379 - Flags: review?(mkanat) → review-
(Assignee)

Comment 8

8 years ago
Created attachment 428384 [details] [diff] [review]
v2 -- without POD
Attachment #428379 - Attachment is obsolete: true
Attachment #428384 - Flags: review?(mkanat)
(Reporter)

Comment 9

8 years ago
Comment on attachment 428384 [details] [diff] [review]
v2 -- without POD

>=== modified file 'Bugzilla/WebService.pm'
> sub datetime_format {
>     my ($date_string) = @_;

  We might as well refactor the JSONRPC implementation of this to just call this one and add a Z on the end, now.

  In fact, maybe we should standardize bz_convert_datetime even?

>=== modified file 'Bugzilla/WebService/Bugzilla.pm'
>+    $db_time = datetime_from($db_time, 'UTC');
>     my $now_utc = DateTime->now();
> 
>-    my $tz = Bugzilla->local_timezone;
>-    my $now_local = $now_utc->clone->set_time_zone($tz);
>-    my $tz_offset = $tz->offset_for_datetime($now_local);
>-
>     return {
>         db_time       => $self->type('dateTime', $db_time),
>-        web_time      => $self->type('dateTime', $now_local),
>+        web_time      => $self->type('dateTime', $now_utc),

  Hmm. We're passing the DateTime objects directly to type(), it looks like. I think that it should expect and allow that, and not try to do datetime_from again on them.

>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'
>+        # We only support UTC TimeZones inbound
>+        if ($value !~ /T.*[-+Z]/i) {

  I think - might still need to be escaped there? Not sure. Might want to do it just to be safe. Does + need to be escaped as well?
Attachment #428384 - Flags: review?(mkanat) → review-
(Assignee)

Comment 10

8 years ago
Created attachment 429026 [details] [diff] [review]
v3 -- refactored, without POD

still need help with POD wording, and which POD's specifically to modify
Attachment #428384 - Attachment is obsolete: true
Attachment #429026 - Flags: review?(mkanat)
(Reporter)

Comment 11

8 years ago
Comment on attachment 429026 [details] [diff] [review]
v3 -- refactored, without POD

>=== modified file 'Bugzilla/WebService.pm'
>+    my $time = $date;
>+    if blessed($date) {

  In Perl, this syntax always needs () around it.

  I wonder if we should rename this to outbound_datetime_format, or something, and name the other one inbound_datetime_parse. (Or datetime_format_outbound and datetime_format_inbound--that might be better.)

>=== modified file 'Bugzilla/WebService/Bugzilla.pm'
>=== modified file 'Bugzilla/WebService/Server.pm'
>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>     elsif ($type eq 'dateTime') {
>         # ISO-8601 "YYYYMMDDTHH:MM:SS" with a literal T
>-        $retval = $self->datetime_format($value);
>+        # YUI expects ISO8601 in UTC time; including TZ specifier
>+        $retval = $self->datetime_format($value) . 'Z';

  Instead of that, override datetime_format to add the Z itself. That might be useful if we start to unify more of the architecture of the system.

>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'
>+        if ($value !~ /T.*[\-+Z]/i) {
>+           # The caller did not specify a timezone we assume UTC.

  "timezone, so we assume"

  This looks good otherwise. Now it just needs POD.
Attachment #429026 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

8 years ago
Created attachment 429376 [details] [diff] [review]
v4 - final code changes; POD phase 1

if this POD needs adjusting can you please just come right out and tell me what POD you want? ;-)

[I took the liberty of Deprecating Bugzilla::Time]
Attachment #429026 - Attachment is obsolete: true
Attachment #429376 - Flags: review?(mkanat)
(Reporter)

Comment 13

8 years ago
Comment on attachment 429376 [details] [diff] [review]
v4 - final code changes; POD phase 1

>=== modified file 'Bugzilla/WebService.pm'
> A hash with a single item, C<timezone>, that is the timezone offset as a
> string in (+/-)XXXX (RFC 2822) format.
> 
>+=item B<History>
>+
>+=over
>+
>+=item As of Bugzilla 3.6 this was hardcoded to be treated as UTC rather
>+than the server's actual timezone. For backwards compatability you should
>+use this return value if you need the specific time.


  Remove the second sentence, because timezone() is deprecated, so you shouldn't be using the return value at all. The first sentence should have added at the end:

  ", because Bugzilla now returns all times from the WebService in UTC."

  Also, bold "3.6".

> =item C<time>
> 
>-B<UNSTABLE>
>+B<DEPRECATED>

  Actually, it's not deprecated. This was originally added so that external clients could know what time the server *thought* it was, even if it wasn't right. So this should still stay.

>@@ -256,6 +262,12 @@
> 
> =item Added in Bugzilla B<3.4>.
> 
>+=item Deprecated in Bugzilla B<3.6>.
>+
>+=item As of Bugzilla 3.6 the values here are all treated as if the server is in UTC.
>+If you wish to be backwards compatible you must review the relevant parts of this interface
>+and calculate accordingly.

  Instead of this note (which is somewhat still required, just to note the UTC change), the individual pieces of this POD (as to what each return item is) should be updated to be accurate.

>@@ -306,6 +309,10 @@
> C<dateTime.iso8601> fields. Bugzilla does--it treats empty values as
> C<undef> (called C<NULL> or C<None> in some programming languages).
> 
>+Bugzilla also accepts a Timezone specifier for C<dateTime.iso8601> fields,
>+but never outputs one. If a TZ specifier is omitted, the passed in time is
>+assumed to be UTC.

  Cool, pretty good. Just a few small fixes:

Bugzilla also accepts a timezone specifier at the end of C<dateTime.iso8601>
fields that are specified as method arguments. The format of the timezone
specifier is specified in the ISO-8601 standard. If no timezone specifier
is included, the passed-in time is assumed to be in the UTC timezone.
Bugzilla will never output a timezone specifier on returned data, because
doing so would violate the XML-RPC specification. All returned times are in
the UTC timezone.
Attachment #429376 - Flags: review?(mkanat) → review-
(Assignee)

Comment 14

8 years ago
(In reply to comment #13)
> (From update of attachment 429376 [details] [diff] [review])
> >@@ -256,6 +262,12 @@
> > 
> > =item Added in Bugzilla B<3.4>.
> > 
> >+=item Deprecated in Bugzilla B<3.6>.
> >+
> >+=item As of Bugzilla 3.6 the values here are all treated as if the server is in UTC.
> >+If you wish to be backwards compatible you must review the relevant parts of this interface
> >+and calculate accordingly.
> 
>   Instead of this note (which is somewhat still required, just to note the UTC
> change), the individual pieces of this POD (as to what each return item is)
> should be updated to be accurate.
> 

I'm not sure I parse this... but your not in IRC to clarify now, I have everything else done, this we just need to touch base on (easier to comment than post a patch that will be r- because I didn't address this ;-) )
(Reporter)

Comment 15

8 years ago
Created attachment 430210 [details] [diff] [review]
POD

Here's the POD. I assume that since it's just POD, it's OK for me to r+ this myself.
Attachment #430210 - Flags: review+
(Assignee)

Comment 16

8 years ago
Created attachment 430220 [details] [diff] [review]
v4 -- final, no POD

review requested for sake of argument.

This was a hand edited patch from my current state of my tree, if this doesn't apply I'll mod the files in-place for you and generate the diff correctly.
Attachment #429376 - Attachment is obsolete: true
Attachment #430220 - Flags: review?(mkanat)
(Reporter)

Updated

8 years ago
Attachment #430220 - Flags: review?(mkanat) → review+
(Reporter)

Updated

8 years ago
Flags: approval3.6+
Flags: approval+
(Assignee)

Comment 17

8 years ago
There, pushed live

http://bzr.mozilla.org/bugzilla/3.6/revision/7011

and

http://bzr.mozilla.org/bugzilla/trunk/revision/7042

Though my methods might need a little tweaking in future.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 550618
You need to log in before you can comment on or make changes to this bug.