265188 - GenerateVersionTable should only overwrite versioncache if it's changed

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Dave Miller [:justdave]]

To aide browser caching, we should compare the newly-generated temp versioncache
file with the previous version if one exists, and if they match, delete the temp
version instead of moving it overtop of the original.  This leaves the inode and
last-modified stamp of the versioncache file alone, which allows apache to not
tell browsers that the file has changed.

Comment 1

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

this patch is only half tested, so please test it too before reviewing it.

Comment 3

[Frédéric Buclin]

checksetup.pl and doeditparams.cgi remove versioncache in all cases, which
sounds safe and good to me. Updating a product using editproducts.cgi does it
too, which may be useless if the product hasn't been changed, but this file is
such a mess (for example, some validations occur *after* the DB has been
updated) that I prefer to let him remove it in all cases for now.

So my patch fixes editclassifications.cgi only which is the single file (except
editproducts.cgi) to remove versioncache in all cases.

Note that my patch also removes the "update versioncache every hour" which is
useless as we already know that versioncache wouldn't be altered.

Comment 4

[:glob ✱]

Comment on attachment 188944 [details] [diff] [review]
patch, v1

r=glob

i'm 100% not sure that removing the CleanTokenTable is a good idea but it
doesn't belong in GenerateVersionTable, and that interval is an overkill.

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 188944 [details] [diff] [review]
patch, v1

Something about this is eating at my conscience for some reason....  I want to
poke at this some myself before I approve it...

Comment 6

[Max Kanat-Alexander]

Comment on attachment 188944 [details] [diff] [review]
patch, v1

Yeah, you definitely can't remove CleanTokenTable. That's quite important. :-)

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> (From update of attachment 188944 [details] [diff] [review] [edit])
> Yeah, you definitely can't remove CleanTokenTable. That's quite important. :-)
> 

mkanat, could you please ask me or think a bit more why I removed
CleanTokenTable before denying review? I would really appreciate.


(23:33:01) LpSolit: myk: bug 23067 is the bug which added CleanTokenTable into
globals.pl
(23:37:26) LpSolit: myk: see justdave's comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=23067#c22
(23:37:56) LpSolit: address would get different token numbers, to prevent them
from being able to  easily masquerade as the opposite address when clicking the
link.  They would  obviously be tied by the userID in the token records. 
Instead of installing Yet  Another Cron Job to check when the end of the 72
hours has ocurred, the check  could be hacked to run any time the versioncache
file is updated (which ocurrs  once and hour or once per Bugzilla access,
whichever is longer). 
(23:39:06) myk: right
(23:41:17) myk: LpSolit: given that, it seems like it would be sufficient to
clean the token table every time someone runs token.cgi
(23:41:43) myk: LpSolit: in fact, it seems like it makes sense to clean the
tokens table only every once in a while
(23:44:15) justdave: it might be worth pointing out that we had to move the
cleaning of the logincookies table out to a cron job on bmo
(23:45:04) justdave: it normally runs every time someone logs in, and it made
logins really slow here
(23:45:40) myk: right; and it doesn't need to be run that often


mkanat, I ask you to reconsider your review, but this time not based on your
feeling that "it may perhaps eventually be bad to remove CleanTokenTable from
globals.pl". Thanks!

Comment 8

[Max Kanat-Alexander]

OK. If you want to remove CleanTokenTable, though, I think we could do it in
another bug.

At the least, your patch has to put CleanTokenTable *somewhere*.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> At the least, your patch has to put CleanTokenTable *somewhere*.

It's already somewhere:

token.cgi, line 77: Bugzilla::Token::CleanTokenTable();

Comment 10

[Max Kanat-Alexander]

Comment on attachment 188944 [details] [diff] [review]
patch, v1

OK, then. All clear. I'll let justdave take a look at it, too.

Comment 11

[Dave Miller [:justdave]]

Comment on attachment 188944 [details] [diff] [review]
patch, v1

ok, my curiosity is satisfied.	I figured out what was eating at my brain :)  I
was wondering if versioncache stored anything outside of the normal field value
stuff that you could edit, and thus might have stuff in it that didn't get an
unlink when it changed, but everything stored in versioncache is created by one
of the edit* utilities, and they all do the unlink (except for
editclassifications, which you're fixing with this patch).

We should really get rid of all these unlinks and make a
Bugzilla->invalidate_cache() routine (just in case the storage or invalidation
method changes), but that's another bug. :)

Comment 12

[Frédéric Buclin]

Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.13; previous revision: 1.12
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.330; previous revision: 1.329
done