Bug 163290 - move DB handling code into a module

r=justdave, myk, joel, preed
a=justdave

9 files changed, 322 insertions, 198 deletions

diff --git a/Bugzilla.pm b/Bugzilla.pm
index f093edaa5..66831046d 100644
--- a/Bugzilla.pm
+++ b/Bugzilla.pm
@@ -25,6 +25,8 @@ package Bugzilla;
 use strict;
 
 use Bugzilla::CGI;
+use Bugzilla::Config;
+use Bugzilla::DB;
 use Bugzilla::Template;
 
 sub create {
@@ -51,6 +53,26 @@ sub instance {
 
 sub template { return $_[0]->{_template}; }
 sub cgi { return $_[0]->{_cgi}; }
+sub dbh { return $_[0]->{_dbh}; }
+
+sub switch_to_shadow_db {
+    my $self = shift;
+
+    if (!$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};
+}
 
 # PRIVATE methods below here
 
@@ -70,6 +92,9 @@ sub _new_instance {
 sub _init_persistent {
     my $self = shift;
 
+    # We're always going to use the main db, so connect now
+    $self->{_dbh} = $self->{_dbh_main} = Bugzilla::DB::connect_main();
+
     # Set up the template
     $self->{_template} = Bugzilla::Template->create();
 }
@@ -96,6 +121,11 @@ sub DESTROY {
     # may need special casing
     # under a persistent environment (ie mod_perl)
     $self->_cleanup;
+
+    # Now clean up the persistent items
+    $self->{_dbh_main}->disconnect if $self->{_dbh_main};
+    $self->{_dbh_shadow}->disconnect if
+      $self->{_dbh_shadow} and Param("shadowdb")
 }
 
 1;
@@ -189,4 +219,16 @@ The current C<cgi> object. Note that modules should B<not> be using this in
 general. Not all Bugzilla actions are cgi requests. Its useful as a convenience
 method for those scripts/templates which are only use via CGI, though.
 
+=item C<dbh>
+
+The current database handle. See L<DBI>.
+
+=item C<switch_to_shadow_db>
+
+Switch from using the main database to using the shadow database.
+
+=item C<switch_to_main_db>
+
+Change the database object to refer to the main database.
+
 =back
diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
new file mode 100644
index 000000000..4b5d31c14
--- /dev/null
+++ b/Bugzilla/DB.pm
@@ -0,0 +1,258 @@
+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# The Initial Developer of the Original Code is Netscape Communications
+# Corporation. Portions created by Netscape are
+# Copyright (C) 1998 Netscape Communications Corporation. All
+# Rights Reserved.
+#
+# Contributor(s): Terry Weissman <terry@mozilla.org>
+#                 Dan Mosedale <dmose@mozilla.org>
+#                 Jacob Steenhagen <jake@bugzilla.org>
+#                 Bradley Baetz <bbaetz@student.usyd.edu.au>
+#                 Christopher Aillon <christopher@aillon.com>
+
+package Bugzilla::DB;
+
+use strict;
+
+use DBI;
+
+use base qw(Exporter);
+
+%Bugzilla::DB::EXPORT_TAGS =
+  (
+   deprecated => [qw(ConnectToDatabase SendSQL SqlQuote
+                     MoreSQLData FetchSQLData FetchOneColumn
+                     PushGlobalSQLState PopGlobalSQLState)
+                 ],
+);
+Exporter::export_ok_tags('deprecated');
+
+use Bugzilla::Config qw(:DEFAULT :db);
+use Bugzilla::Util;
+
+# 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
+}
+
+# XXX - mod_perl
+my $_current_sth;
+sub SendSQL {
+    my ($str) = @_;
+
+    require Bugzilla;
+
+    $_current_sth = Bugzilla->instance->dbh->prepare($str);
+    return $_current_sth->execute;
+}
+
+# Its much much better to use bound params instead of this
+sub SqlQuote {
+    my ($str) = @_;
+
+    # Backwards compat code
+    return '' if not defined $str;
+
+    require Bugzilla;
+
+    my $res = Bugzilla->instance->dbh->quote($str);
+
+    trick_taint($res);
+
+    return $res;
+}
+
+# XXX - mod_perl
+my $_fetchahead;
+sub MoreSQLData {
+    return 1 if defined $_fetchahead;
+
+    if ($_fetchahead = $_current_sth->fetchrow_arrayref()) {
+        return 1;
+    }
+    return 0;
+}
+
+sub FetchSQLData {
+    if (defined $_fetchahead) {
+        my @result = @$_fetchahead;
+        undef $_fetchahead;
+        return @result;
+    }
+    return $_current_sth->fetchrow_array;
+}
+
+sub FetchOneColumn {
+    my @row = FetchSQLData();
+    return $row[0];
+}
+
+# XXX - mod_perl
+my @SQLStateStack = ();
+
+sub PushGlobalSQLState() {
+    push @SQLStateStack, $_current_sth;
+    push @SQLStateStack, $_fetchahead;
+}
+
+sub PopGlobalSQLState() {
+    die ("PopGlobalSQLState: stack underflow") if ( scalar(@SQLStateStack) < 1 );
+    $_fetchahead = pop @SQLStateStack;
+    $_current_sth = pop @SQLStateStack;
+}
+
+# MODERN CODE BELOW
+
+sub connect_shadow {
+    die "Tried to connect to non-existent shadowdb" unless Param('shadowdb');
+
+    my $dsn = "DBI:mysql:host=" . Param("shadowdbhost") .
+      ";database=" . Param('shadowdb') . ";port=" . Param("shadowdbport");
+
+    $dsn .= ";mysql_socket=" . Param("shadowdbsock") if Param('shadowdbsock');
+
+    return _connect($dsn);
+}
+
+sub connect_main {
+    my $dsn = "DBI:mysql:host=$::db_host;database=$::db_name;port=$::db_port";
+
+    $dsn .= ";mysql_socket=$::db_sock" if $::db_sock;
+
+    return _connect($dsn);
+}
+
+sub _connect {
+    my ($dsn) = @_;
+
+    # connect using our known info to the specified db
+    # Apache::DBI will cache this when using mod_perl
+    my $dbh = DBI->connect($dsn,
+                           $db_user,
+                           $db_pass,
+                           { RaiseError => 1,
+                             PrintError => 0,
+                             HandleError => \&_handle_error,
+                             FetchHashKeyName => 'NAME_lc',
+                             TaintIn => 1,
+                           });
+
+    return $dbh;
+}
+
+sub _handle_error {
+    require Carp;
+
+    $_[0] = Carp::longmess($_[0]);
+    return 0; # Now let DBI handle raising the error
+}
+
+1;
+
+__END__
+
+=head1 NAME
+
+Bugzilla::DB - Database access routines, using L<DBI>
+
+=head1 SYNOPSIS
+
+  my $dbh = Bugzilla::DB->connect_main;
+  my $shadow = Bugzilla::DB->connect_shadow;
+
+  SendSQL("SELECT COUNT(*) FROM bugs");
+  my $cnt = FetchOneColumn();
+
+=head1 DESCRIPTION
+
+This allows creation of a database handle to connect to the Bugzilla database.
+This should never be done directly; all users should use the L<Bugzilla> module
+to access the current C<dbh> instead.
+
+Access to the old SendSQL-based database routines are also provided by
+importing the C<:deprecated> tag. These routines should not be used in new
+code.
+
+=head1 CONNECTION
+
+A new database handle to the required database can be created using this
+module. This is normally done by the L<Bugzilla> module, and so these routines
+should not be called from anywhere else.
+
+=over 4
+
+=item C<connect_main>
+
+Connects to the main database, returning a new dbh.
+
+=item C<connect_shadow>
+
+Connects to the shadow database, returning a new dbh. This routine C<die>s if
+no shadow database is configured.
+
+=back
+
+=head1 DEPRECATED ROUTINES
+
+Several database routines are deprecated. They should not be used in new code,
+and so are not documented.
+
+=over 4
+
+=item *
+
+ConnectToDatabase
+
+=item *
+
+SendSQL
+
+=item *
+
+SqlQuote
+
+=item *
+
+MoreSQLData
+
+=item *
+
+FetchSQLData
+
+=item *
+
+FetchOneColumn
+
+=item *
+
+PushGlobalSQLState
+
+=item *
+
+PopGlobalSQLState
+
+=back
+
+=head1 SEE ALSO
+
+L<DBI>
+
+=cut
diff --git a/buglist.cgi b/buglist.cgi
index 118d542c9..10e659a1f 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -35,6 +35,7 @@ use lib qw(.);
 
 use vars qw($cgi $template $vars);
 
+use Bugzilla;
 use Bugzilla::Search;
 
 # Include the Bugzilla CGI and general utility library.
@@ -627,7 +628,7 @@ if ($serverpush) {
 
 # Connect to the shadow database if this installation is using one to improve
 # query performance.
-ReconnectToShadowDatabase();
+Bugzilla->instance->switch_to_shadow_db();
 
 # Normally, we ignore SIGTERM and SIGPIPE (see globals.pl) but we need to
 # respond to them here to prevent someone DOSing us by reloading a query
@@ -685,11 +686,6 @@ while (my @row = FetchSQLData()) {
     push(@bugidlist, $bug->{'bug_id'});
 }
 
-# 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();
-
 # Check for bug privacy and set $bug->{isingroups} = 1 if private 
 # to 1 or more groups
 my %privatebugs;
diff --git a/checksetup.pl b/checksetup.pl
index 68789f393..e4d610b53 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -222,11 +222,11 @@ my $modules = [
     }, 
     { 
         name => 'DBI', 
-        version => '1.13' 
+        version => '1.32' 
     }, 
     { 
         name => 'DBD::mysql', 
-        version => '1.2209' 
+        version => '2.1010' 
     }, 
     { 
         name => 'File::Spec', 
diff --git a/collectstats.pl b/collectstats.pl
index 1f8b4783c..27a6e1840 100755
--- a/collectstats.pl
+++ b/collectstats.pl
@@ -31,6 +31,8 @@ use vars @::legal_product;
 
 require "globals.pl";
 
+use Bugzilla;
+
 # tidy up after graphing module
 if (chdir("graphs")) {
     unlink <./*.gif>;
@@ -38,9 +40,11 @@ if (chdir("graphs")) {
     chdir("..");
 }
 
-ConnectToDatabase(1);
+ConnectToDatabase();
 GetVersionTable();
 
+Bugzilla->instance->switch_to_shadow_db();
+
 my @myproducts;
 push( @myproducts, "-All-", @::legal_product );
 
diff --git a/duplicates.cgi b/duplicates.cgi
index 5687cefec..45eb219ff 100755
--- a/duplicates.cgi
+++ b/duplicates.cgi
@@ -34,6 +34,7 @@ require "CGI.pl";
 
 use vars qw($buffer);
 
+use Bugzilla;
 use Bugzilla::Search;
 use Bugzilla::CGI;
 
@@ -50,11 +51,13 @@ if ($::FORM{'ctype'} && $::FORM{'ctype'} eq "xul") {
 # Use global templatisation variables.
 use vars qw($template $vars);
 
-ConnectToDatabase(1);
+ConnectToDatabase();
 GetVersionTable();
 
 quietly_check_login();
 
+Bugzilla->instance->switch_to_shadow_db();
+
 use vars qw (%FORM $userid @legal_product);
 
 my %dbmcount;
diff --git a/globals.pl b/globals.pl
index 28e065f98..5ad624f9a 100644
--- a/globals.pl
+++ b/globals.pl
@@ -28,6 +28,7 @@
 
 use strict;
 
+use Bugzilla::DB qw(:DEFAULT :deprecated);
 use Bugzilla::Constants;
 use Bugzilla::Util;
 # Bring ChmodDataFile in until this is all moved to the module
@@ -97,7 +98,6 @@ $::SIG{PIPE} = 'IGNORE';
 
 $::defaultqueryname = "(Default query)"; # This string not exposed in UI
 $::unconfirmedstate = "UNCONFIRMED";
-$::dbwritesallowed = 1;
 
 #sub die_with_dignity {
 #    my ($err_msg) = @_;
@@ -106,175 +106,6 @@ $::dbwritesallowed = 1;
 #}
 #$::SIG{__DIE__} = \&die_with_dignity;
 
-sub ConnectToDatabase {
-    my ($useshadow) = (@_);
-    $::dbwritesallowed = !$useshadow;
-    $useshadow &&= Param("shadowdb");
-    my $connectstring;
-
-    if ($useshadow) {
-        if (defined $::shadow_dbh) {
-            $::db = $::shadow_dbh;
-            return;
-        }
-        $connectstring="DBI:mysql:host=" . Param("shadowdbhost") .
-          ";database=" . Param('shadowdb') . ";port=" . Param("shadowdbport");
-        if (Param("shadowdbsock") ne "") {
-            $connectstring .= ";mysql_socket=" . Param("shadowdbsock");
-        }
-    } else {
-        if (defined $::main_dbh) {
-            $::db = $::main_dbh;
-            return;
-        }
-        $connectstring="DBI:mysql:host=$::db_host;database=$::db_name;port=$::db_port";
-        if ($::db_sock ne "") {
-            $connectstring .= ";mysql_socket=$::db_sock";
-        }
-    }
-    $::db = DBI->connect($connectstring, $::db_user, $::db_pass)
-      || die "Bugzilla is currently broken. Please try again " .
-        "later. If the problem persists, please contact " .
-        Param("maintainer") . ". The error you should quote is: " .
-        $DBI::errstr;
-
-    if ($useshadow) {
-        $::shadow_dbh = $::db;
-    } else {
-        $::main_dbh = $::db;
-    }
-}
-
-sub ReconnectToShadowDatabase {
-    if (Param("shadowdb")) {
-        ConnectToDatabase(1);
-    }
-}
-
-sub ReconnectToMainDatabase {
-    if (Param("shadowdb")) {
-        ConnectToDatabase();
-    }
-}
-
-# This is used to manipulate global state used by SendSQL(),
-# MoreSQLData() and FetchSQLData().  It provides a way to do another
-# SQL query without losing any as-yet-unfetched data from an existing
-# query.  Just push the current global state, do your new query and fetch
-# any data you need from it, then pop the current global state.
-# 
-@::SQLStateStack = ();
-
-sub PushGlobalSQLState() {
-    push @::SQLStateStack, $::currentquery;
-    push @::SQLStateStack, [ @::fetchahead ]; 
-}
-
-sub PopGlobalSQLState() {
-    die ("PopGlobalSQLState: stack underflow") if ( $#::SQLStateStack < 1 );
-    @::fetchahead = @{pop @::SQLStateStack};
-    $::currentquery = pop @::SQLStateStack;
-}
-
-sub SavedSQLStates() {
-    return ($#::SqlStateStack + 1) / 2;
-}
-
-
-my $dosqllog = (-e "data/sqllog") && (-w "data/sqllog");
-
-sub SqlLog {
-    if ($dosqllog) {
-        my ($str) = (@_);
-        open(SQLLOGFID, ">>data/sqllog") || die "Can't write to data/sqllog";
-        if (flock(SQLLOGFID,2)) { # 2 is magic 'exclusive lock' const.
-
-            # if we're a subquery (ie there's pushed global state around)
-            # indent to indicate the level of subquery-hood
-            #
-            for (my $i = SavedSQLStates() ; $i > 0 ; $i--) {
-                print SQLLOGFID "\t";
-            }
-
-            print SQLLOGFID time2str("%D %H:%M:%S $$", time()) . ": $str\n";
-        }
-        flock(SQLLOGFID,8);     # '8' is magic 'unlock' const.
-        close SQLLOGFID;
-    }
-}
-
-sub SendSQL {
-    my ($str) = (@_);
-
-    # Don't use DBI's taint stuff yet, because:
-    # a) We don't want out vars to be tainted (yet)
-    # b) We want to know who called SendSQL...
-    # Is there a better way to do b?
-    if (is_tainted($str)) {
-        die "Attempted to send tainted string '$str' to the database";
-    }
-
-    my $iswrite =  ($str =~ /^(INSERT|REPLACE|UPDATE|DELETE)/i);
-    if ($iswrite && !$::dbwritesallowed) {
-        die "Evil code attempted to write '$str' to the shadow database";
-    }
-
-    # If we are shutdown, we don't want to run queries except in special cases
-    if (Param('shutdownhtml')) {
-        if ($0 =~ m:[\\/]((do)?editparams.cgi)$:) {
-            $::ignorequery = 0;
-        } else {
-            $::ignorequery = 1;
-            return;
-        }
-    }
-    SqlLog($str);
-    $::currentquery = $::db->prepare($str);
-    if (!$::currentquery->execute) {
-        my $errstr = $::db->errstr;
-        # Cut down the error string to a reasonable.size
-        $errstr = substr($errstr, 0, 2000) . ' ... ' . substr($errstr, -2000)
-                if length($errstr) > 4000;
-        die "$str: " . $errstr;
-    }
-    SqlLog("Done");
-}
-
-sub MoreSQLData {
-    # $::ignorequery is set in SendSQL
-    if ($::ignorequery) {
-        return 0;
-    }
-    if (defined @::fetchahead) {
-        return 1;
-    }
-    if (@::fetchahead = $::currentquery->fetchrow_array) {
-        return 1;
-    }
-    return 0;
-}
-
-sub FetchSQLData {
-    # $::ignorequery is set in SendSQL
-    if ($::ignorequery) {
-        return;
-    }
-    if (defined @::fetchahead) {
-        my @result = @::fetchahead;
-        undef @::fetchahead;
-        return @result;
-    }
-    return $::currentquery->fetchrow_array;
-}
-
-
-sub FetchOneColumn {
-    my @row = FetchSQLData();
-    return $row[0];
-}
-
-    
-
 @::default_column_list = ("bug_severity", "priority", "rep_platform", 
                           "assigned_to", "bug_status", "resolution",
                           "short_short_desc");
@@ -1347,22 +1178,6 @@ sub SplitEnumType {
     return @result;
 }
 
-
-# This routine is largely copied from Mysql.pm.
-
-sub SqlQuote {
-    my ($str) = (@_);
-#     if (!defined $str) {
-#         confess("Undefined passed to SqlQuote");
-#     }
-    $str =~ s/([\\\'])/\\$1/g;
-    $str =~ s/\0/\\0/g;
-    # If it's been SqlQuote()ed, then it's safe, so we tell -T that.
-    trick_taint($str);
-    return "'$str'";
-}
-
-
 # UserInGroup returns information aboout the current user if no second 
 # parameter is specified
 sub UserInGroup {
diff --git a/report.cgi b/report.cgi
index f727ee466..d113e6d89 100755
--- a/report.cgi
+++ b/report.cgi
@@ -28,6 +28,8 @@ require "CGI.pl";
 
 use vars qw($cgi $template $vars);
 
+use Bugzilla;
+
 # Go straight back to query.cgi if we are adding a boolean chart.
 if (grep(/^cmd-/, $cgi->param())) {
     my $params = $cgi->canonicalise_query("format", "ctype");
@@ -44,7 +46,7 @@ GetVersionTable();
 
 confirm_login();
 
-ReconnectToShadowDatabase();
+Bugzilla->instance->switch_to_shadow_db();
 
 my $action = $cgi->param('action') || 'menu';
 
diff --git a/reports.cgi b/reports.cgi
index e18d3ee37..230fe32db 100755
--- a/reports.cgi
+++ b/reports.cgi
@@ -51,13 +51,17 @@ $@ && ThrowCodeError("chart_lines_not_installed");
 my $dir = "data/mining";
 my $graph_dir = "graphs";
 
+use Bugzilla;
+
 # If we're using bug groups for products, we should apply those restrictions
 # to viewing reports, as well.  Time to check the login in that case.
-ConnectToDatabase(1);
+ConnectToDatabase();
 quietly_check_login();
 
 GetVersionTable();
 
+Bugzilla->instance->switch_to_shadow_db();
+
 # We only want those products that the user has permissions for.
 my @myproducts;
 push( @myproducts, "-All-");