Closed Bug 139632 Opened 23 years ago Closed 23 years ago

ConnectToDatabase misuse - partial fix for 2.16

Categories

(Bugzilla :: Bugzilla-General, defect)

2.15
x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: bbaetz, Assigned: bbaetz)

References

Details

Attachments

(1 file)

We call ConnectToDatabse() in a whole lot of places. Instead, we should only do so at the top of scripts. Currently, we have the problem noted on landfill where if you try to enter a bug when you're not logged in, and we're scheduled rebuild the versioncache, then we clear the token table first. Rebuilding the versioncache calls ConnectToDB, and if you're logged in with a non-0 groupset we call userInGroup, which also connects to the db. The token code doesn't, and so we fail when making a db query to do the updates This is thus a 2.16 blocker. The easy solution is just to always connectToDatabase at the top of scripts, and never anywhere else. This will cause a sperious connection on pages which don't otherwise need the versioncache, when a person isn't logged in (wince then we wouldn't need them for the footer, either). I think that the product selection part of enter_bug.cgi is the only one of those we have, so I think we can live with that... This blocks the replication bug, bug 124589, because there I've changed ConnectToDatabase() to mean 'connect to the real db' instead of the current behaviour of returning if we already have a connection. Yes, I could make ConnectoToDatabase with no args only connect if we're not connected, but I think that its easier to just remove the calls from helper functions, which are scattered thoguhtout the code, as seen from the current issue with enter_bug. MattyT has longer-term plans for a transaction API. Those shouldn't affect this, though.
I'll hack on this later tonight, if I get time.
Target Milestone: --- → Bugzilla 2.16
Since it's possible to tell if there were any arguments at all passed, I think it would be safer (and probably more useful in general to prevent spurious connections) if we make ConnectToDatabase() return if we have a connection, ConnectToDatabase(0) connect to the real database, and ConnectToDatabase(1) connect to the shadow database.
This is probably against comment #2, but what if we take a slightly different approach to what I wanted on IRC? We make ConnectToDatabase (or some appropriately renamed sub) be called at the top of every script, specifying which database to use, but it doesn't do the actual connection, this is done by SendSQL when needed, to the database specified by ConnectToDatabase. This way we get laziness in database connection, and we avoid specifying the database to SendSQL and avoiding the problems that would cause.
$::selecteddb = 0; sub SelectDatabase ($) { ($::selecteddb) = @_; } sub SendSQL ... { my $db; if ($::selectddb == 0) { if ($::maindbconn is not connected) { connect $::maindbconn; } $db = $::maindbconn; } else if ($::selecteddb == 1) { if ($::shadowdbconn is not connected) { connect $::shadowdbconn; } $db = $::shadowdbconn; } else { error ... } use the $db variable ... ... }
So what's the fix for the token code? If this is only a blocker because of that problem, this bug should only be about fixing that problem. Otherwise this isn't a blocker.
This problem was initially discovered on query.cgi but could not be later reproduced. The causes of that could be totally different. We also don't know how many of these problems exist in the new templatised code. We only know how to fix them.
What mattyt said. However, an acceptable 2.16 fix would just be to make sure that connectToDatabase is called from every cgi. We can handle cleaning up the mess later. enter_bug.cgi is the only major cgi which doesn't call it always, and I'd guess that we'd trigger that code only 5 times per hour at most, which as I said above should be ok, I feel. quips.cgi is the same as enter_bug in that the db isn't used, but that overhead is almost certainly none, since bmo doesn't allow people to enter quips anyway. It doesn't use the versioncache though, either, so it won't try to be rebuilt. xml.cgi also doesn't call connectToDatabase explicitly, but it has to connect to the db. doeditparams doesn't, but it calls confirm_login, which connects. Most of the edit* stuff has the same thing, as does userprefs.cgi. index.cgi doesn't need the db if you're not logged in, but it does connect. Thats certainly more overhead than enter_bug. Note that any page with a footer will connect to the database if you have a cookie (via GetCommandMenu), so we're only discussing cases where people arne't logged in at all - was mistaken in my previous comment, and 100 is probably a high number. So, for 2.16 I suggest adding ConnectToDatabase calls to those cgis which don't call them currently. enter_bug.cgi is the only blocker for this, though. We can remove the uneeded ones post-release.
Comment on attachment 80840 [details] [diff] [review] patch v1: fixes blocker problem 2xr=gerv. Gerv
Attachment #80840 - Flags: review+
Blocker resolved, reducing severity. Checking in enter_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v <-- enter_bug.cgi new revision: 1.65; previous revision: 1.64 done
Severity: blocker → minor
Pushing out to 2.18. The major problem is solved. Gerv
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
I don't want this on the "ready to check in" list for 2.18 if it's not ready to check in. Let's open a new bug to finish this.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Summary: ConnectToDatabase misuse → ConnectToDatabase misuse - partial fix for 2.16
Target Milestone: Bugzilla 2.18 → Bugzilla 2.16
Rule 1: Don't resolve partial bugs as fixed unless you've already opened another bug.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: