281360 - Checksetup should use the new DB-compatibility layer to create $dbh

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Max Kanat-Alexander]

Right now, we still use DBI directly to create the $dbh handle inside of
checksetup. Instead, we should create a Bugzilla::DB object with the new
DB-compatibility layer.

Later, checksetup will be modified to fully support DB-independence.

But this change here blocks all that work, so it needs to go in first.

Comment 1

[Max Kanat-Alexander]

Attachment: Make checksetup use Bugzilla::DB and move checksetup's errors there (use -p1)

OK, here is a patch to do this. It requires the DBCompat patch, of course (bug
237862). It's pretty simple, and I think it's a nicer way to do things, anyhow.
:-)

Comment 2

[Tomas Kopal]

(In reply to comment #1)
> Created an attachment (id=173621) [edit]
> Make checksetup use Bugzilla::DB and move checksetup's errors there (use -p1)
> 
> OK, here is a patch to do this. It requires the DBCompat patch, of course (bug
> 237862). It's pretty simple, and I think it's a nicer way to do things, anyhow.
> :-)

I haven't tested this, so I am just guessing here :-), but I think this will not
work. Note that checksetup.pl is constructing the DSN from different variables
($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port
etc.).
DB.pm is using Config.pm, which in turn is using localconfig file, which, by the
time checksetup is run for the first time, does not have to exists yet. You need
some other way how to get the parameters from checksetup.pl to DB.pm, without
using localconfig.
I would probably go for adding parameters to Bugzilla->dbh(). If specified, they
will override the localconfig parameters (and will be specified only from
checksetup.pl).

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> I haven't tested this, so I am just guessing here :-), but I think this will not
> work. Note that checksetup.pl is constructing the DSN from different variables
> ($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port
> etc.).
> DB.pm is using Config.pm, which in turn is using localconfig file, which, by the
> time checksetup is run for the first time, does not have to exists yet.

  Notice that checksetup is also getting all those files from localconfig. At
tihs point in checksetup, localconfig is guaranteed to exist, so we're safe to
use Config.pm.

  The only thing that I can think of that I might actually have to change is
doing a specific "require" on Bugzilla::DB. But I'll probably just end up doing
that when the globals.pl require is removed.

Comment 4

[Tomas Kopal]

(In reply to comment #3)
> (In reply to comment #2)
> > I haven't tested this, so I am just guessing here :-), but I think this will not
> > work. Note that checksetup.pl is constructing the DSN from different variables
> > ($my_db_driver <> $db_driver; $my_db_host <> $db_host; $my_db_port <> $db_port
> > etc.).
> > DB.pm is using Config.pm, which in turn is using localconfig file, which, by the
> > time checksetup is run for the first time, does not have to exists yet.
> 
>   Notice that checksetup is also getting all those files from localconfig. At
> tihs point in checksetup, localconfig is guaranteed to exist, so we're safe to
> use Config.pm.
> 
>   The only thing that I can think of that I might actually have to change is
> doing a specific "require" on Bugzilla::DB. But I'll probably just end up doing
> that when the globals.pl require is removed.

Well, you are right :-). The my_ variables were there just to prevent compiler
errors, but as we are not using these parameters here anymore (they are used in
Bugzilla::DB), compiler is happy :-).
Maybe you can now remove $my_db_host, $my_db_port, $my_db_user, $my_db_sock and
$my_db_pass variables, as they are not used anymore...

Comment 5

[Max Kanat-Alexander]

Attachment: Version 2 (use -p1)

OK! I removed the no-longer-used $my variables from checksetup.

I also discovered that checksetup would fail if it had to create the database,
with my previous patch.

I've fixed that by allowing a method of connect_main that doesn't connect to a
database. It's documented that it should only be used for creating the
database. :-)

Comment 6

[Tomas Kopal]

(In reply to comment #5)
> Created an attachment (id=173738) [edit]
> Version 2 (use -p1)
> 
> OK! I removed the no-longer-used $my variables from checksetup.
> 
> I also discovered that checksetup would fail if it had to create the database,
> with my previous patch.
> 
> I've fixed that by allowing a method of connect_main that doesn't connect to a
> database. It's documented that it should only be used for creating the
> database. :-)

Nice, good catch. Two comments:
 - Nit: shouldn't the parameter be a constant? ;-).
 - This will not work on Postgres. AFAIK you can't connect to a server and not
to any DB. With Pg you are usually solving this by conecting to the system
database, template1. We need to handle this in the DB specific code, but I
assume this can be done by checking if the name is empty, and substituing
"template1" in the Pg.pm module...

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
>  - Nit: shouldn't the parameter be a constant? ;-).

  Hahahhaa. Yeah, I thought about that. I only use it in ONE place, though,
ever. I thought it would be a waste of space to make it a constant. :-)

>  - This will not work on Postgres. AFAIK you can't connect to a server and not
> to any DB. With Pg you are usually solving this by conecting to the system
> database, template1. We need to handle this in the DB specific code, but I
> assume this can be done by checking if the name is empty, and substituing
> "template1" in the Pg.pm module...

  Oh, right... the problem is that the "bugs" user might not have access to the
"template1" or "template0" database. (I'd probably use "template0," since that
would more closely approximate the behavior of MySQL, where I can't do anything
when I'm not connected to a DB [template0 is usually read-only].)

  However, DBI might handle the situation for us, also. So we'll have to see, in
an actual test...

Comment 8

[Max Kanat-Alexander]

*** Bug 281364 has been marked as a duplicate of this bug. ***

Comment 9

[Tomas Kopal]

One more idea came over my mind: maybe you could call directly _connect function
from the checksetup module, that way you can pass any parameters you want (it
will be $my_ version again I guess). It would make the code maybe a bit cleaner
than using the optional parameter which shouldn't be used otherwise. That also
means _connect would no longer be private, but if properly documented that is
not an issue.
Take this just as an idea, not a review point, and ignore it if you please :-).

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> One more idea came over my mind: maybe you could call directly _connect function
> from the checksetup module, that way you can pass any parameters you want (it
> will be $my_ version again I guess). It would make the code maybe a bit cleaner
> than using the optional parameter which shouldn't be used otherwise. That also
> means _connect would no longer be private, but if properly documented that is
> not an issue.

  Yeah. I thought about that, too. I figured that _connect should stay private.
That way I could take those localconfig variables out of checksetup, which I
very much wanted to be able to do. The fewer localconfig variables that I need
in checksetup, the happier I am.

Comment 11

[Max Kanat-Alexander]

Oops, this actually blocks checked-in patches in a fashion that we didn't
realize -- you can't call the Bugzilla::DB functions on $dbh because it's still
DBI, not Bugzilla::DB. I KNEW there was a reason that I originally marked this
as a blocker to checksetup changes.

Comment 12

[Joel Peshkin]

Comment on attachment 173738 [details] [diff] [review]
Version 2 (use -p1)

Change the connect_main(1); to something more like connect_main("dont actually
connect here"); or something more explanatory than "1" and r=joel

Comment 13

[Max Kanat-Alexander]

Attachment: v2

Cool. OK, I integrated joel's suggestion and another good suggestion from glob,
which was to also display $DBI::errstr if we fail connect.

Comment 14

[Max Kanat-Alexander]

Attachment: v2.1

Removed connection string, because DBI is now giving us the error message quite
clearly.

Comment 15

[:glob ✱]

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

r=glob

Comment 16

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.346; previous revision: 1.345
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.21; previous revision: 1.20
done