284525 - Checksetup uses some bad SQL that is not cross-database compatible

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Max Kanat-Alexander]

There are a few places that PostgreSQL chokes in checksetup code, before it can
even get to the bz_ functions in Bugzilla::DB. These need to be fixed before I
can fix each of those individual bz_ functions to be cross-database compatible.

Comment 1

[Max Kanat-Alexander]

Attachment: Fix bad quoting and insertion of "NULL" as a string

These are simple fixes. I've verified that they work on Pg and on MySQL.

Comment 2

[:glob ✱]

Comment on attachment 176083 [details] [diff] [review]
Fix bad quoting and insertion of "NULL" as a string

>-$sth = $dbh->prepare("SELECT description FROM products");
>+my $sth = $dbh->prepare("SELECT description FROM products");

this causes
"my" variable $sth masks earlier declaration in same scope at checksetup.pl
line 2386.


otherwise looks good.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> this causes
> "my" variable $sth masks earlier declaration in same scope at checksetup.pl
> line 2386.

  Oh, that's part of the patch from bug 284172. :-) Oops. :-)

Comment 4

[Tomas Kopal]

Comment on attachment 176083 [details] [diff] [review]
Fix bad quoting and insertion of "NULL" as a string

>Index: checksetup.pl
>@@ -2230,23 +1651,21 @@
> sub AddFDef ($$$) {
>     my ($name, $description, $mailhead) = (@_);
> 
>-    $name = $dbh->quote($name);
>-    $description = $dbh->quote($description);

Are you sure it's safe to leave this out? Can't the function be ever called
with user-supplied or otherwise unsafe data? (That's not suggestion, that's
really just question :-) ).

>@@ -2384,20 +1804,26 @@
> ###########################################################################
> # Create initial test product if there are no products present.
> ###########################################################################
>-$sth = $dbh->prepare("SELECT description FROM products");
>+my $sth = $dbh->prepare("SELECT description FROM products");

I remember this change was already in another patch :-).

>     $dbh->do("INSERT INTO fielddefs " .
...
>+    $dbh->do(q{INSERT INTO products(name, description, milestoneurl, 
...
>+    $dbh->do(qq{INSERT INTO versions (value, product_id) 

Can't we somehow make all the quoting more consistent? We are using single
quotes, double quotes, q{}, qq{} all over the code... But thats probably more
question for some general rules than for this patch...

Otherwise, it looks good...

Comment 5

[:glob ✱]

> Are you sure it's safe to leave this out? Can't the function be ever called
> with user-supplied or otherwise unsafe data? (That's not suggestion, that's
> really just question :-) ).

it's save; using sql placeholders automatically performs the quoting.

Comment 6

[Max Kanat-Alexander]

Attachment: v2

OK, this one's all better. :-)

Comment 7

[:glob ✱]

Comment on attachment 176226 [details] [diff] [review]
v2

r=glob

Comment 8

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.359; previous revision: 1.358
done