163290 - Move DB handling code into a module

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Bradley Baetz (:bbaetz)]

I've started hacking on this. It makes lots of stuff a lot cleaner.

Comment 1

[Bradley Baetz (:bbaetz)]

First draft coming up.

I got rid of the sqlstatestack, so I updated those methods to use the DBI stuff.

The rest needs to wait, though - see discussion on developers for more.

Comment 2

[Bradley Baetz (:bbaetz)]

Attachment: v 0.1

OK, try this. Suggested ordering:

- Bugzilla/DBI.pm
- Other .pms (most of which are just removing &::'s)
- Other CGIs

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: ok, try this

Mattyt wanted the backend first. Here it is....

Comment 4

[Matthew Tuck [:CodeMachine]]

Comments:

- When this goes in, need a summary of why we can't use placeholders for the
devguide (old shadow impl, whatever taint problem there was).
- Also the comment about never using placeholders doesn't say why.
- Preferably update release notes to account for new versions in this patch.
- You say quote will not detaint eventually, need a comment to the effect it is
backward compat, at the point where it detaints.

I haven't checked this for implementation equivalence yet but what's there looks OK.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: take 2

Now that we don't have to support handling the shadowdb ourself, we don't need
to inherit from the DBI stuff at all.

Please note the circular dependancies between Bugzilla and Bugzilla::DB. In
this case, these are probably avoidable. In general, they won't be (eg Bugzilla
will do user logins, but the login code will want to use templates via
Bugzilla->instance->template). Brief discussion on IRC suggested that that was
ok, though.

I thought about putting the switch_* stuff in Bugzilla::DB, but I'm trying to
keep all the pereistent objects in the one place (Bugzilla.pm).

Also, note the updated module dependancies - updated DBI is needed for taint
support and updated mysql is needed for correctly escaping stuff via bind
params (according to Changelog). As usual, its reccommented to update DBI
first, then recompile DBD::msyql against the newer Driver.xst

Comment 6

[Gervase Markham [:gerv]]

I can't review this until after Christmas (I get back on Dec 29th) and probably
not until the New Year. Just so you know :-)

Gerv

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 109990 [details] [diff] [review]
take 2

>@@ -51,6 +53,26 @@
> 
> sub template { return $_[0]->{_template}; }
> sub cgi { return $_[0]->{_cgi}; }
>+sub dbh { return $_[0]->{_dbh}; }

I don't really like this whole switch_to_shadow_db thing; I think there should
be a
sub shadow_dbh { ... }, and methods that use the shadow db should call/use that
to get a handle.

However, I'm going to assume this was done for a (backwards compatibility?)
reason... so if that's the case, ignore me. But I still think there should be a
sub shadow_db that returns the "shadowdb" handle OR null if the param is off.

Actually, in looking at it more, I guess it's necessary because we use all
those homegrown DB access methods instead of the standard DBI methods... hrm...

>+sub switch_to_shadow_db {
>+    my $self = shift;
>+
>+    unless ($self->{_dbh_shadow}) {
>+        if (Param('shadowdb')) {
>+            $self->{_dbh_shadow} = Bugzilla::DB::connect_shadow();
>+        } else {
>+            $self->{_dbh_shadow} = $self->{_dbh_main};
>+        }
>+    }
>+
>+    $self->{_dbh} = $self->{_dbh_shadow};
>+}
>+
>+sub switch_to_main_db {
>+    my $self = shift;
>+    $self->{_dbh} = $self->{_dbh_main};
>+}

Ick.

>-
>-# Switch back from the shadow database to the regular database so PutFooter()
>-# can determine the current user even if the "logincookies" table is corrupted
>-# in the shadow database.
>-ReconnectToMainDatabase();

Do we no longer have to switch back because we're using the DB replication,
which should replicate the logincookies table?

I don't know how I feel about that... but that may be because I'm not sure what
the update time on MySQL's replication is...

>+# All this code is backwards compat fu. As such, its a bit ugly. Note the
>+# circular dependancies on Bugzilla.pm
>+# This is old cruft which will be removed, so theres not much use in
>+# having a separate package for it, or otherwise trying to avoid the circular
>+# dependancy
>+
>+sub ConnectToDatabase {
>+    # We've already been connected in Bugzilla.pm
>+}

If this is just a stub function anyway, why not blow all the:

-ConnectToDatabase(1);
+ConnectToDatabase();

away from the .cgis? This initially confused me because I thought with the
Bugzilla object they all have, they'd already be connected to the DB. Now I'm
*really confused. :-)

>+sub PopGlobalSQLState() {
>+    die ("PopGlobalSQLState: stack underflow") if ( $#SQLStateStack < 1 );

This should be "if ( $#SQLStateStack < 0 )", since when $#SQLStateStack = 0,
that means it still has one element in it.

BUT, that notation is ass; you should use "if (scalar(@SQLStateStack) < 1);"

>+sub _connect {

I'd like a more descriptive name here, only because DBI's method is also called
connect, and when I saw _connect above, I was kinda scratching my head. Maybe
bz_dbconnect or something?

Comment 8

[Bradley Baetz (:bbaetz)]

>I don't really like this whole switch_to_shadow_db thing; I think there should be
>a sub shadow_dbh { ... }, and methods that use the shadow db should call/use that
>to get a handle.

But we can't, and its not just for backwards compat. Consider a common
subroutine, such as get_product_id. That can be called from someone using the
shadowdb (such as Bugzilla::Search, via buglist.cgi) or someone using the main
db (process_bug.cgi). The subroutine needs to be able to use whatever database
handle is 'current'.

We then get:

sub get_product_id {
  my $prod = shift;
  return $Bugzilla->dbh->selectrow_array("SELECT id FROM products WHERE name =
?", undef, $prod);
}

(subs which have more than one db access can obviously store Bugzilla->dbh in a
local var, and then use $dbh instead. Also, get_product_id should probably be
caching the statement handle, since this sub is called often, but thats not the
issue here)

Note that I haven't made it possible for someone to get access to a specific
dbh, (eg to run a single statement). I can do that easily if someone ever needs
that, though.

> Ick

Why? the only vaguely complecated code is to avoid having two db conectsion when
we're not using a shadowdb, and thats just a param check, which has been
simplified by teh fact that we don't suppport USE to switch between dbs (since
they're on different servers with mysql)

>Do we no longer have to switch back because we're using the DB replication,
>which should replicate the logincookies table?

No, we no longer have to switch back because we do the calculations up front.
Pre-templating, the footer code used PutFooter, which called get_command_menu.
get_command_menu did quietly_check_login using the shadowdb, so if the
logincookie hadn't propogated from teh main db (whcih is still possible with
replication, since its async) then the login failed, and you were logged out.
'Corruption' had nothign to do with it.

Now we calculate all of this in the login object, and set $vars->{user} to hold
the info. There is no db access below that line any more (well, there is, but
thats for post processing of query results, and so should be using the same db
anyway)

Note that hte problem from that original bug would have shown on all scripts
using the shadowdb, of which we now have several. So if there was an issue, we
should have seen it on those pages.

> <Blow away CtD calls>

Well, my first round didn't. I'd like to leave those in place though, at least
until we finalise what the code will be doing for 'startup' with and without
mod_perl - it may turn out to be needed. Teh removal of the '1' is because we
should always be using the main db for authentication. Bugzilla->create will
eventually take a hashref to override its behaviour for auto-connection, but the
only user of that will be collectstats.pl. For now, it just does an extra
uneeded db connection.

I'll thnk about it, though, and probably will end up removing it

> <SQLStateStack>

I just copied the original :) Will fix.

> <sub _connect>

Well, the leading _ is for an internal method. Remember that this is actuall
Bugzilla::DB::_connect, so there aren't any namespace issues here. _connect is
effectivly 'our' DBI connect wrapper.

Comment 9

[J. Paul Reed [:preed]]

re comment 8: ok, makes sense about the switching, but I still don't like how
you've done it.

I would do this:

-- have a main_db method, which returns the main db handle
-- have a shadow_db method, which returns the shadow_db handle, or die()s if
there is no shadow_db
-- have a current_db method, which returns the "current" db handle.
-- have switch_to_* methods, which switches what current_db gives you

The reason I think this is worth noting is because I think it's important to be
able to get to the underlying (main or shadow) handles directly if you need to,
and I just don't like that you can't do that here.

I see why you're doing what you're doing, but I think a set of methods, like
those above, will make it clearer why that's necessary, and which handle you're
actually getting in client code (the 'current' one, the main one, or the shadow
one).

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 109990 [details] [diff] [review]
take 2

joel? Also see comments in the bug...

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 109990 [details] [diff] [review]
take 2

Argh, dependencies.

>Index: Bugzilla.pm

>@@ -51,6 +53,26 @@

>+    unless ($self->{_dbh_shadow}) {

Nit: this is more logical as "if (!$self->{_dbh_shadow})" when read as english.

r=myk.

Maybe I'm missing something, Joel, but what important thing does this code not
allow you to do?

Comment 12

[Joel Peshkin]

myk: in comment 11, what are you talking about?

Comment 13

[Joel Peshkin]

Comment on attachment 109990 [details] [diff] [review]
take 2

Fix the kooky confusing expression in the PopGlobalSQLState() underrun check
and r=joel

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 109990 [details] [diff] [review]
take 2

Please don't remove sub die_with_dignity().  Even though it's commented out, I
uncomment that frequently (to get confess) when trying to track down bugs with
elusive error messages.

With that fixed, r=justdave

Comment 15

[Bradley Baetz (:bbaetz)]

justdave: Bet you haven't tried it recently....

It doesn't work under perl 5.8, or apache2. (errors which are then caught via
eval don't work) Its going to have to go when we remove the file anyway

DB errors can be caught via handle_error

See that Bugzilla::exception mail I sent a while back. I'll keep it, but not
forever...

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: v2.1

OK, here we go. Jaypee, is this still ok? I left CtD for a later cleanup, and I
think I convicend you on irc why your suggestion wasn't useful, but was trivial
to add later if anyone did come up with a correct rationale.

Comment 17

[Bradley Baetz (:bbaetz)]

Comment on attachment 111493 [details] [diff] [review]
v2.1

I think I can carry forward the other reviews - jp?

Comment 18

[J. Paul Reed [:preed]]

Comment on attachment 111493 [details] [diff] [review]
v2.1

>+sub dbh { return $_[0]->{_dbh}; }

We kinda talked about this already, but I still think there should be dbh_main
and dbh_shadow accessor methods that do the right thing (tm) (for dbh_shadow,
die() if there is no shadow db, etc.), but we talked about this below and on
IRC, and your reasons for leaving them out are as good as my reasons for
putting them in, so...

r=preed

BTW, I'd just like to point out: this code is really cool.

Comment 19

[Dave Miller [:justdave]]

"Make it so"

Comment 20

[Bradley Baetz (:bbaetz)]

Fix checked in