533330 - JSON-RPC webservice returns and expects a bad date format

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Max Kanat-Alexander]

Right now we're returning dates in timestamp format, I believe, but we should instead be returning dates in some sort of ISO 8601 format, which is what YUI parses as dates in JSON (and which is a way better date format anyway).

Comment 1

[Max Kanat-Alexander]

Also, we need to accept dates in that format, instead of timestamps.

Comment 2

[Justin Wood (:Callek)]

Attachment: v1 -- Just copy XMLRPC implementation

This looks like it should do it; not yet tested (feel free to beat me to the punch there, I'll need much more of my time to do testing).

Diff is against bzr trunk, if it matters

Comment 3

[Max Kanat-Alexander]

Comment on attachment 425138 [details] [diff] [review]
v1 -- Just copy XMLRPC implementation

  Sweet! :-) Comments below:

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>+        # ISO-8601 "YYYYMMDDTHH:MM:SS" with a literal T
>+        $retval = datetime_format($value);

  Probably want to do $self->datetime_format().

>+sub datetime_format {
>+    my $iso_datetime = sprintf('%d%02d%02dT%02d:%02d:%02d',
>+        $year + 1900, $mon + 1, $mday, $hour, $min, $sec);
> [snip]
>+    # extensions to the XML-RPC date standard.
>+    $time =~ /^(\d{4})(\d{2})(\d{2})T(\d{2}):(\d{2}):(\d{2})/;

  I don't think we're going to be using the "T" format. Also, I think we may want to include (or at least accept) timezones. There's some particular format that YUI actually generates and accepts in some places, and what's what I want to use.

Comment 4

[Justin Wood (:Callek)]

Attachment: v2, use UTC/GMT

From looking at YUI docs, it expects the 'T' format with 'Z' at end (for UTC).

This is again untested, though confidence is slightly down from last patch.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 425539 [details] [diff] [review]
v2, use UTC/GMT

>+sub datetime_format {
>+    my ($self, $date_string) = @_;
>+
>+    my $time = str2time($date_string);
>+    my ($sec, $min, $hour, $mday, $mon, $year) = gmtime $time;

  I think it would be safer to use datetime_from, from Bugzilla::Util, and then call set_timezone('UTC') (I think that's the syntax) on the resulting datetime object, and then use the strftime accessor to format it.

  DateTime.pm is a little more reliable in terms of conversions than any internal stuff.

> sub _bz_convert_datetime {
>     my ($self, $time) = @_;
>-    my $dt = DateTime->from_epoch(epoch => ($time / 1000), 
>-                                  time_zone => Bugzilla->local_timezone);
>-    return $dt->strftime('%Y-%m-%d %T');
>+    # We leave off the $ from the end of this regex to allow for possible
>+    # extensions to the date standard.
>+    $time =~ /^(\d{4})(\d{2})(\d{2})T(\d{2}):(\d{2}):(\d{2})Z/;
>+    $time = "$1-$2-$3 $4:$5:$6 UTC";

  I don't think that the "UTC" will work on the average MySQL installation, which is what we care about the most. Probably what we need to do here is to create a DateTime object and then get the right local time out of it.

Comment 6

[Justin Wood (:Callek)]

Attachment: v3; use DateTime directly

Hopefully better; again untested

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 425550 [details] [diff] [review]
v3; use DateTime directly

Note I have not yet updated the POD; which will be part of this bug.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 425550 [details] [diff] [review]
v3; use DateTime directly

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>+    
>+    my $converted = datetime_from($time);

  There's one thing we need to test here--does $converted actually contain the UTC timezone, now? That is, does strptime understand that "Z" means UTC?

  BTW, you can pass Bugzilla->local_timezone as the second argument to datetime_from to have it do the conversion internally. (It will also prevent it from doing an additional conversion to Bugzilla->user->timezone before you get it back.)

>+    $time = $converted->ymd('-') . ' ' . $converted->hms();

  Docs say that "ymd" defaults to "-", so we don't need to specify it.

Comment 9

[Max Kanat-Alexander]

Oh, and to be clear--I'll r- or r+ the code once we know the answer to my question, there.

Comment 10

[Justin Wood (:Callek)]

(In reply to comment #8)
> (From update of attachment 425550 [details] [diff] [review])
> >=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
> >+    
> >+    my $converted = datetime_from($time);
> 
>   There's one thing we need to test here--does $converted actually contain the
> UTC timezone, now? That is, does strptime understand that "Z" means UTC?

Well, in testing $time is _read_ as UTC correctly from the format: "YYYYMMDDThh:mm:ssZ" but $converted will already contain the Bugzilla->local_timezone; yay!

>   BTW, you can pass Bugzilla->local_timezone as the second argument to
> datetime_from to have it do the conversion internally. (It will also prevent it
> from doing an additional conversion to Bugzilla->user->timezone before you get
> it back.)
> 
> >+    $time = $converted->ymd('-') . ' ' . $converted->hms();
> 
>   Docs say that "ymd" defaults to "-", so we don't need to specify it.

I'll fix this sure;

NOTE: I should also be able to streamline earlier code by simply passing the UTC as the second param to datetime_from.

If this all, with my notes looks good; I'll update one last time, and include POD too.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> Well, in testing $time is _read_ as UTC correctly from the format:
> "YYYYMMDDThh:mm:ssZ" but $converted will already contain the
> Bugzilla->local_timezone; yay!

  Okay. And when you print out the time, the values have correctly been converted from UTC to local?

> NOTE: I should also be able to streamline earlier code by simply passing the
> UTC as the second param to datetime_from.

  Ah, perhaps, yeah.

> If this all, with my notes looks good; I'll update one last time, and include
> POD too.

  Yeah, sounds good.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 425550 [details] [diff] [review]
v3; use DateTime directly

Marking this r- so that I know we're waiting for a new patch.

Comment 13

[Justin Wood (:Callek)]

Attachment: v4 -- hopefully final

(In reply to comment #11)
> (In reply to comment #10)
> > Well, in testing $time is _read_ as UTC correctly from the format:
> > "YYYYMMDDThh:mm:ssZ" but $converted will already contain the
> > Bugzilla->local_timezone; yay!
> 
>   Okay. And when you print out the time, the values have correctly been
> converted from UTC to local?

Correct.

If the POD doesn't match your preference, feel free to quote a new POD to put there and I can fix that on checkin. [I'm not the best with writing documentation]

Comment 14

[Max Kanat-Alexander]

Comment on attachment 425680 [details] [diff] [review]
v4 -- hopefully final

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
>+C<dateTime> fields are the standard C<iso8601>  field. They should be
>+in C<YYYY-MM-DDTHH:MM:SSZ> format (where C<T> and C<Z> is a literal T
>+and Z respectively). We accept non UTC times on any value passed in
>+(localtime assumed if no TZ specified), however all outgoing times are in UTC
>+(specified with the Z).


C<dateTime> fields are strings in the standard ISO-8601 format: C<YYYY-MM-DDTHH:MM:SSZ>, where C<T> and C<Z> are a literal T and Z, respectively. The "Z" means that all times are in UTC timezone--times are always returned in UTC, and should be passed in as UTC. (Note: The JSON-RPC interface currently also accepts non-UTC times for any values passed in, if they include a time-zone specifier that follows the ISO-8601 standard, instead of "Z" at the end. This behavior is expected to continue into the future, but to be fully safe for forward-compatibility with all future versions of Bugzilla, it is safest to pass in all times as UTC with the "Z" timezone specifier.)


  Also, this patch seems to have the same patch in it three times, and I think only the bottom one is the right one?

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #14)
> (From update of attachment 425680 [details] [diff] [review])
> >=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'
> >+C<dateTime> fields are the standard C<iso8601>  field. They should be
> >+in C<YYYY-MM-DDTHH:MM:SSZ> format (where C<T> and C<Z> is a literal T
> >+and Z respectively). We accept non UTC times on any value passed in
> >+(localtime assumed if no TZ specified), however all outgoing times are in UTC
> >+(specified with the Z).
> 
> 
> C<dateTime> fields are strings in the standard ISO-8601 format:...

Sure.

>   Also, this patch seems to have the same patch in it three times, and I think
> only the bottom one is the right one?

Erm it does; thats a stupidity artifact from me using |bzr diff >> ../| rather than simply |>|

Comment 16

[Justin Wood (:Callek)]

Attachment: v5 -- use better POD

Comment 17

[Max Kanat-Alexander]

Comment on attachment 425745 [details] [diff] [review]
v5 -- use better POD

> ##################
> # Login Handling #
> ##################
>@@ -162,9 +170,10 @@
> 
> sub _bz_convert_datetime {
>     my ($self, $time) = @_;
>-    my $dt = DateTime->from_epoch(epoch => ($time / 1000), 
>-                                  time_zone => Bugzilla->local_timezone);
>-    return $dt->strftime('%Y-%m-%d %T');
>+    
>+    my $converted = datetime_from($time);

  Ahh, here you do have to do Bugzilla->local_timezone as the second argument, because otherwise you might sometimes get the time back in Bugzilla->user->timezone.

Comment 18

[Justin Wood (:Callek)]

Attachment: v6 -- last time

as far as I saw that this was actually not needed; but it does make things more clear (even if it is not needed). So no problem. This should do it!

Comment 19

[Max Kanat-Alexander]

Comment on attachment 425919 [details] [diff] [review]
v6 -- last time

Yeah, it is needed, otherwise it will come out in $user->timezone if the user's timezone is set to something that isn't the local timezone.

This looks good! :-) Thank you so much, Callek!

Comment 20

[Justin Wood (:Callek)]

Max, I'm not yet understanding enough of bzr branches/[whatever they called there] to trust myself pushing to the stable branch; I can do the trunk checkin in about 24 hours though, but if you want to checkin to trunk for me I don't object.

Comment 21

[Frédéric Buclin]

We can help once you are on IRC. Ping me or Max.

Comment 22

[Frédéric Buclin]

Comment on attachment 425919 [details] [diff] [review]
v6 -- last time

> use Date::Parse;
> use DateTime;

On checkin, please remove these two lines. AFAICS, they are now useless as your patch removes str2time($value) and my $dt = DateTime->from_epoch(...). Or attach a new patch and carry forward mkanat's r+.

Comment 23

[Justin Wood (:Callek)]

Attachment: Patch as pushed

Comment 24

[Justin Wood (:Callek)]

$ bzr commit --fixes=mozilla:533330 -F ../bug-commit.txt
No handlers could be found for logger "bzr"
Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified Bugzilla/WebService/Server/JSONRPC.pm
Committed revision 6970.

Comment 25

[Justin Wood (:Callek)]

I also pushed to trunk; but accidentally lost the bzr commit message;.