515515 - For clients, mid-air collision results when user's timezone preference differs from server's

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Robert Elves]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 3.4.1

If the user has changed their timezone, the Mylyn Bugzilla Connector receives a localized delta_ts within the bug's xml. Upon posting a change, this localized delta_ts is not being interpreted server side, resulting in a mid-air collision response.

Reproducible: Always

Steps to Reproduce:
1. Change time zone to something different than the server
2. Post a change from Mylyn's task editor
3. Collision occurs

Comment 1

[Robert Elves]

With a differing time zone selected, the web ui contains:

   <input type="hidden" name="delta_ts" value="2009-09-09 12:53:06">

XML contains:

   2009-09-09 09:53:06 -0700

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

This patch adds the local time as an attribute to both <creation_ts> and <delta_ts>. This way, you get e.g. something like:

<delta_ts local_time="2009-10-19 21:28:18">2009-10-20 08:28:18 +1300</delta_ts>

local_time contains the correctly formatted local time and can be used as is to submit changes to process_bug.cgi. And the time in the user timezone is still passed as it was before so that it can be used in the GUI.

Robert, can you make sure it fixes your problem?

Comment 3

[Gervase Markham [:gerv]]

Can we instead have an unambiguous time - i.e. in UTC? That can be converted to the user's local time zone for display, or to Bugzilla's time zone for resubmission. 

If the client does not know Bugzilla's timezone, and you don't want to make them make a webservices call to find it out, then the time in Bugzilla's timezone (for submission to process_bug.cgi) could be a second provided timestamp. But the way this is going, it looks like we are eventually going to have to provide three, because neither of the two in the current plan is the UTC time.

I seem to remember mkanat saying he wanted Bugzilla interfaces to return UTC. But I may have misquoted or misremembered him. Max?

Gerv

Comment 4

[Max Kanat-Alexander]

Yes, Bugzilla should always return UTC, but that's a breaking change, an enhancement, so it would be something for HEAD. I'd also like Bugzilla to always store UTC in the database, but that's another breaking change.

Comment 5

[Frédéric Buclin]

Using UTC is out of the scope of this bug. What I did is to provide an easy way to get a working timestamp to submit back to process_bug.cgi. There is no reason for now to give the time in UTC and ask the client to convert it to the timezone of the server, especially if the client hasn't this information. I'm currently fixing a bug, not doing refactoring.

Comment 6

[Frédéric Buclin]

No news from Robert, so it's going to miss 3.4.3.

Comment 7

[Robert Elves]

Sorry for the slow response here. This patch would still require changes in our code base to perform special handling when the additional local_time attribute is present.  Is there any way this could simply be resolved through proper interpretation of the date server side upon post and if no offset is found then use the servers time zone?

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> is present.  Is there any way this could simply be resolved through proper
> interpretation of the date server side upon post and if no offset is found then
> use the servers time zone?

This is doable. This would mean some extra work for process_bug.cgi, though, which I wanted to avoid. I will give it a look, but this will be too invasive/risky for Bugzilla 3.4.x.

Comment 9

[Gervase Markham [:gerv]]

The current patch is certainly better than nothing :-). What else is needed to get it on to the 3.4 branch (and, ideally, into 3.4.3)? Let's not let the best be the enemy of the good.

Gerv

Comment 10

[Gervase Markham [:gerv]]

Robert: I fixed this in the BzAPI proxy as follows:

- The BzAPI proxy knows the timezone of the Bugzilla. (I did it as a config option, but you can also get this info from the XML-RPC interface.)

- Times which come in are converted to UTC - either using the attached offset if there is one, or by using our knowledge of the timezone.

- Times which go out are converted back again using our knowledge of the timezone.

Can you use a similar mechanism to solve this problem for your users without needing any XML changes?

However, I'm also requesting approval, as this bug has one positive review, and the freeze is tomorrow.

Gerv

Comment 11

[Robert Elves]

(In reply to comment #10)
> - The BzAPI proxy knows the timezone of the Bugzilla. (I did it as a config
> option, but you can also get this info from the XML-RPC interface.)

Unfortunately both options aren't ideal for us in the short term.

(In reply to comment #2)
> Created an attachment (id=407083)
> patch, v1
> 
> This patch adds the local time as an attribute to both <creation_ts> and
> <delta_ts>. This way, you get e.g. something like:
> 
> <delta_ts local_time="2009-10-19 21:28:18">2009-10-20 08:28:18 +1300</delta_ts>

How about we go ahead with this patch but reverse the fields to retain backwards compatibility?  I.e. the delta_ts element value holds whatever is usually presented in the html and accepted upon submit, while the attribute "localized_time" be set to the user's local offset. I.e.:

<delta_ts localized_time="2002-04-16 16:49:17 -0400">2002-04-16 16:49:17</delta_ts> 

Thoughts?

Comment 12

[Gervase Markham [:gerv]]

A patch which kept existing values the same as now and just added new ones is, I would suspect, far more likely to get accepted.

Gerv

Comment 13

[Gervase Markham [:gerv]]

Robert: are you in a position to revise the patch?

Gerv

Comment 14

[Robert Elves]

Attachment: revised patch

This patch implements what I outlined in comment#10. For consistency, the creation_ts should likely also be presented in the repos local time within the element and the localized version in the attribute as it is.  I'm not sure how to get the correctly formatted creation time delta_ts worked, creation_ts had an alternate format.

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 414782 [details] [diff] [review]
revised patch

Hi Robert,

This doesn't look right. I thought we were going to retain backward compatibility for the "val", and put the new value you wanted in an attribute? But here, it seems like the value for "val" is changing:

>-    [% ELSIF field == 'creation_ts' OR field == 'delta_ts' %]
>-      [% val = val FILTER time("%Y-%m-%d %T %z") %]

...

>+      [% val = val FILTER time("%Y-%m-%d %T", Bugzilla.local_timezone) %]

...

>+      [% val = bug.delta_ts %]

Gerv

Comment 16

[Robert Elves]

My intent is to have the element value be what old bugzillas would have returned (the delta_ts that results in successful submission) and have the attribute value be the new localized version.

Comment 17

[Gervase Markham [:gerv]]

Robert: unless I've missed something, that's not what your patch does. It changes the value of "val" from

val FILTER time("%Y-%m-%d %T %z")

to

val FILTER time("%Y-%m-%d %T", Bugzilla.local_timezone)

Are those always the same? If so, why the change?

Gerv

Comment 18

[Robert Elves]

I expect that the creation timestamp val should be derived from the bug similarly to how I did the delta_ts. The format was different though when I tried so figured somebody might know how to get at it in a proper consistent format. The val of the delta_ts is my primary concern.

Comment 19

[Gervase Markham [:gerv]]

Forgive me for being dense, but I still don't get it.

A design goal is not to change the value ("val") of the text between the two tags in any case, but to add a new attribute to the tag giving the value you would prefer to have access to.

If that is true, why do you need to change the line
      [% val = val FILTER time("%Y-%m-%d %T %z") %]
to anything else at all? That's the bit you want to keep the same, right?

Gerv

Comment 20

[Gervase Markham [:gerv]]

Robert: LpSolit's original patch v.1 looks right to me, although I would prefer that the new attribute also contained its own timezone information, thereby making it unambiguous, and allowing conversion to UTC if necessary without needing to make another call to find out the Bugzilla.timezone from another source.

Gerv

Comment 21

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Like this, in fact. (Same patch as LpSolit's v1 plus a %z).

Gerv

Comment 22

[Robert Elves]

 > Are those always the same? If so, why the change?

My intent was just to have the element value be what was historically the same value that gets transmitted in the html version's delta_ts hidden attribute. All our (Mylyn) existing clients that use the delta_ts value upon submitting would then still work as expected.

Comment 23

[Gervase Markham [:gerv]]

Max: do we want the value which was always there to:

a) be the (wrong?) value it always was (Gerv's patch)
or
b) be the value people probably expected it to be (Robert's patch)

a) could be argued to be "most backwardly compatible" in theory but b) could be argued to be most backwardly compatible in practice with existing clients - Mylyn in particular.

Gerv

Comment 24

[Max Kanat-Alexander]

Well, I think that the primary data in the XML should reflect exactly what is displayed on the show_bug page in HTML, which is the simplest solution for most clients. I think that the UTC timestamp should be an attribute, even though this is more difficult for clients who need the UTC data.

The WebService should eventually solve this problem by always exposing data as UTC.

The XML element data should contain a timezone, though, if feasible. Of course, an Olsen timezone is the most accurate, but for parseability, I suspect an offset is instead the most consumable.

Comment 25

[Gervase Markham [:gerv]]

(In reply to comment #24)
> Well, I think that the primary data in the XML should reflect exactly what is
> displayed on the show_bug page in HTML, which is the simplest solution for most
> clients.

Well, the show_bug page in HTML cuts off the seconds values and uses the ambiguous timezone identifiers (BST, PST). I presume you don't want the XML to do the same?

> I think that the UTC timestamp should be an attribute, even though
> this is more difficult for clients who need the UTC data.

In my current patch, the Bugzilla-local timestamp is the attribute, but of course because TZ information is attached, you can always convert it to a UTC timestamp. Providing a UTC timestamp, however, would not allow you to do the reverse conversion, and therefore we would have two times provided and _neither_ would be the one you need to send back as delta_ts! :-)

> The WebService should eventually solve this problem by always exposing data as
> UTC.
> 
> The XML element data should contain a timezone, though, if feasible.

It already does, so that's OK :-)

So, it looks like my patch v.2 is the way to go. Sorry, Robert, it looks like you are going to need to issue updates to old versions of Mylyn for those users who want to set their displayed timezone in the UI. :-|

Gerv

Comment 26

[Max Kanat-Alexander]

(In reply to comment #25)
> Well, the show_bug page in HTML cuts off the seconds values and uses the
> ambiguous timezone identifiers (BST, PST). I presume you don't want the XML to
> do the same?

  Correct, I do not want the XML to do the same. :-)

> In my current patch, the Bugzilla-local timestamp is the attribute, but of
> course because TZ information is attached, you can always convert it to a UTC
> timestamp. Providing a UTC timestamp, however, would not allow you to do the
> reverse conversion, and therefore we would have two times provided and
> _neither_ would be the one you need to send back as delta_ts! :-)

  Having the local time in the element with a timezone is what I want. Then there should also be a UTC time in an attribute, since DateTime within Bugzilla is more suited to calculate the UTC time than a client would be, I believe. I haven't looked at your patch, so if that's what it does, then that's cool. :-)

Comment 27

[Gervase Markham [:gerv]]

Max: sorry to go on about this, but you may not realise it, but you are saying contradictory - or at least, ambiguous - things :-)

At the moment, the *element* contains the time converted to the timezone requested by the user, with a timezone identifier to indicate this, and there is no *attribute*. The problem is that if the user changes the timezone they are requesting to a different one to Bugzilla's timezone, it's not possible to work out the correct value to submit back as delta_ts, because the timezone of the server is no longer known.

My patch does not change the *element* at all - i.e. it keeps it as what show_bug displays, which is what you say you want. But this is not necessarily compatible with your statement "Having the local time in the element with a timezone is what I want." Do you mean server-local or user-local?

My patch adds a new *attribute*, which always contains the server-local time with a timezone. From either the attribute or the element, it is possible to work out the UTC time, but neither contains the UTC time directly. This is done because converting from local to UTC is easy, but UTC to local is impossible (as you don't know the right timezone). This is possibly incompatible with your statment "there should also be a UTC time in an attribute" - as in, it's possible to calculate the UTC time from the attribute, but the attribute is not directly the UTC time value. But we have to do it that way, for the reasons given in this paragraph.

So is that patch, which does what I outline above, OK? Yes or no? :-)

Gerv

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> converting from local to UTC is easy, but UTC to local is impossible
> (as you don't know the right timezone).

  Ah, that's all you needed to say. That's true, and that's fine, then.

Comment 29

[Gervase Markham [:gerv]]

OK, great. ghendricks: are you able to review this patch, or should I ask someone else?

Gerv

Comment 30

[Robert Elves]

Great to see more discussion here! Gerv, where does this leave my need for a submittable 'delta_ts' in the xml?

Comment 31

[Gervase Markham [:gerv]]

Robert: it's the value of the "local_time" attribute on the tag, converted to a time_t.

Gerv

Comment 32

[Robert Elves]

(In reply to comment #31)
> Robert: it's the value of the "local_time" attribute on the tag, converted to a
> time_t.
> 
> Gerv

Thanks Gerv, this should work from our side!

Comment 33

[Max Kanat-Alexander]

Comment on attachment 419013 [details] [diff] [review]
Patch v.2

>+      [% attr_name = "local_time" %]

  That should be server_time instead--I think that's clearer.


  But you know what? I just read this bug over again, and we absolutely should be taking into account the timezone of the submitted delta_ts--that would solve the whole problem, wouldn't it? I didn't realize that we actually had the timezone in the XML already.

Comment 34

[Robert Elves]

(In reply to comment #33)
>   But you know what? I just read this bug over again, and we absolutely should
> be taking into account the timezone of the submitted delta_ts--that would solve
> the whole problem, wouldn't it? I didn't realize that we actually had the
> timezone in the XML already.

Yes, I believe that is the ideal solution Max.  +1

Comment 35

[Robert Elves]

(In reply to comment #34)
> (In reply to comment #33)
> >   But you know what? I just read this bug over again, and we absolutely should
> > be taking into account the timezone of the submitted delta_ts--that would solve
> > the whole problem, wouldn't it? I didn't realize that we actually had the
> > timezone in the XML already.
> 
> Yes, I believe that is the ideal solution Max.  +1

Further to my +1, is this something that can happen for 3.6?

Comment 36

[Max Kanat-Alexander]

(In reply to comment #35)
> Further to my +1, is this something that can happen for 3.6?

  Heck, if it's not too complicated, I think it's something that can happen in 3.4.

Comment 37

[Gervase Markham [:gerv]]

I'm not sure what you mean by "take into account the timezone of the submitted delta_ts". IIRC, delta_ts, when submitted to process_bug.cgi, is a Unix time_t - i.e. a large integer value. It doesn't have a timezone.

Can someone explain why my analysis in comment #27, which carefully lays out the current situation and says why we need this new value, is wrong?

Gerv

Comment 38

[Robert Elves]

(In reply to comment #27)
> At the moment, the *element* contains the time converted to the timezone
> requested by the user, with a timezone identifier to indicate this, and there
> is no *attribute*. The problem is that if the user changes the timezone they
> are requesting to a different one to Bugzilla's timezone, it's not possible to
> work out the correct value to submit back as delta_ts, because the timezone of
> the server is no longer known.

Right, so if the time coming back in XML has the user's chosen timezone, and the delta_ts in the html also had the users chosen timezone, then all that is required would be for process_bug.cgi to do the timezone conversion back to the repository timezone and from that determine if a mid air collision has in fact occurred.

Comment 39

[Gervase Markham [:gerv]]

OK, I get it. My misunderstanding was that I thought that the delta_ts sent in HTML and resubmitted on change was a Unix time_t (which is always UTC), but in fact it's a timestamp like "2010-02-11 09:31:42". Yes, I agree, if process_bug interpreted that timestamp as in the user's timezone if they'd set one, or otherwise in the server's time zone, it would correctly discern whether there had been a mid-air.

So let's do that :-) It's great that it might be possible to take this in 3.4 as well :-)

Gerv

Comment 40

[Gervase Markham [:gerv]]

Robert: are you able to produce a patch for the solution Max has agreed?

Gerv

Comment 41

[Robert Elves]

(In reply to comment #40)
> Robert: are you able to produce a patch for the solution Max has agreed?
> 
> Gerv

Frank's going to take a pass at the proposed solution.

Comment 42

[Frank Becker]

Attachment: patch, V3

patch for process_bug.cgi tht worhe with and without timezone in delta_ts

On thing that we can optimize is the way how I detect if we had a mid-air collision (local var with 0 or 1) instead of an boolean.

Comment 43

[Max Kanat-Alexander]

Comment on attachment 431201 [details] [diff] [review]
patch, V3

  First, I think I'd like a fix for HEAD that uses datetime_from to generate a DateTime object from everything. DateTime objects can be compared, so you can just do a comparison directly on them. Then we can backport that to 3.4, hopefully still using DateTime to do the comparison.

Comment 44

[Frank Becker]

Attachment: patch, V4

changes from comment#43 are now included

Comment 45

[Max Kanat-Alexander]

Comment on attachment 431295 [details] [diff] [review]
patch, V4

This looks good to me. Look OK to you, LpSolit?

Comment 46

[Frédéric Buclin]

Comment on attachment 431295 [details] [diff] [review]
patch, V4

Yes, seems to work correctly. r=LpSolit

Comment 47

[Frédéric Buclin]

datetime_from() doesn't exist on the 3.4 branch.

Comment 48

[Max Kanat-Alexander]

  LpSolit's comment means that in order for this to be accepted for 3.4, it needs a backport.

Comment 49

[Frank Becker]

(In reply to comment #48)
>   LpSolit's comment means that in order for this to be accepted for 3.4, it
> needs a backport.

Should I do the backport?

Is there something special to know to create a backport?

Comment 50

[Max Kanat-Alexander]

(In reply to comment #49)
> Should I do the backport?

  Yes.

> Is there something special to know to create a backport?

  No, a backport is just a patch that applies and works on a previous branch. In this case, there is no datetime_from on 3.4, so you either have to backport datetime_from (which would be OK) or re-write your patch in some other way.

Comment 51

[Frédéric Buclin]

As we are close from releasing Bugzilla 3.6, I will commit this patch on the 3.6 branch now. We still need a backport for 3.4.7 asap.

Comment 52

[Frédéric Buclin]

Leaving the bug open till we have a patch ready for the 3.4 branch.

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified process_bug.cgi
Committed revision 7105.

Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified process_bug.cgi
Committed revision 7064.

Comment 53

[Max Kanat-Alexander]

  Note that I'm planning to release Bugzilla 3.6 on April 6, at which point the 3.4 series will be locked to security fixes, and any patch on this bug won't be accepted for 3.4, so we'd need a backport before then.

Comment 54

[Frank Becker]

Attachment: patch for 3.4, V5

this is the backport for 3.4.

Sorry for the delay!

Hope the this is not to late.

Comment 55

[Max Kanat-Alexander]

Comment on attachment 436017 [details] [diff] [review]
patch for 3.4, V5

Yeah, that looks great. This is just the same patch with datetime_from added, right?

Comment 56

[Frédéric Buclin]

Comment on attachment 436017 [details] [diff] [review]
patch for 3.4, V5

Sorry, but you mostly duplicated the code from format_time(), meaning we would now have two places to maintain when something goes wrong. format_time() should be fixed accordingly, IMO.

Comment 57

[Max Kanat-Alexander]

Comment on attachment 436017 [details] [diff] [review]
patch for 3.4, V5

No, this is a backport. I don't want to modify format_time, which is used *all over* the Bugzilla code. For a backport, the right thing to do is just to copy in this code and not modify format_time. This is only happening on the 3.4 branch.

Comment 58

[Frank Becker]

(In reply to comment #55)
> (From update of attachment 436017 [details] [diff] [review])
> Yeah, that looks great. This is just the same patch with datetime_from added,
> right?

Yes.

Comment 59

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified process_bug.cgi
modified Bugzilla/Util.pm
Committed revision 6756.