271474 - SQL syntax error when updating max votes per bug in editproducts.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[timeless]

https://bugzilla.mozilla.org/editproducts.cgi?action=edit&product=Documentation
	  	
Updated max votes per bug.
Checking existing votes in this product for anybody who now has too many votes.
Software error:

DBD::mysql::st execute failed: You have an error in your SQL syntax.  Check the
manual that corresponds to your MySQL server version for the right syntax to use
near 'now(),29,'UNCONFIRMED','NEW')' at line 1 [for statement ``INSERT INTO
bugs_activity (bug_id,who,bug_when,fieldid,removed,added) VALUES
(249649,,now(),29,'UNCONFIRMED','NEW')'']) at Bugzilla/DB.pm line 66
	Bugzilla::DB::SendSQL('INSERT INTO bugs_activity
(bug_id,who,bug_when,fieldid,removed,a...') called at CGI.pl line 255
	main::CheckIfVotedConfirmed(249649, undef) called at
/opt/webtools/bugzilla/editproducts.cgi line 1265

For help, please send mail to the webmaster ([e-mail]), giving this
error message and the time and date of the error.

Comment 1

[Kevin Benton]

Change...

(249649,,now(),29,'UNCONFIRMED','NEW')''])

to read...

(249649,0,now(),29,'UNCONFIRMED','NEW')''])

in the code and that should fix it.  Looking at the 2.19.1 DB description, it 
should let it go through, however, without that 0 in there for who, for some 
reason MySQL is rejecting the INSERT.  See MySQL session below:

mysql> INSERT INTO bugs_activity (bug_id,who,bug_when,fieldid,removed,added) 
VALUES (249649,,now(),29,'UNCONFIRMED','NEW');
ERROR 1064: You have an error in your SQL syntax near 'now
(),29,'UNCONFIRMED','NEW')' at line 1
mysql> INSERT INTO bugs_activity (bug_id,who,bug_when,fieldid,removed,added) 
VALUES (249649,0,now(),29,'UNCONFIRMED','NEW');
Query OK, 1 row affected (0.00 sec)

mysql> SELECT * FROM bugs_activity WHERE bug_id = 249649;
+--------+-----------+-----+---------------------+---------+-------+-----------
--+
| bug_id | attach_id | who | bug_when            | fieldid | added | 
removed     |
+--------+-----------+-----+---------------------+---------+-------+-----------
--+
| 249649 |      NULL |   0 | 2004-12-07 13:39:21 |      29 | NEW   | 
UNCONFIRMED |
+--------+-----------+-----+---------------------+---------+-------+-----------
--+
1 row in set (0.00 sec)

Comment 2

[Dave Miller [:justdave]]

(In reply to comment #1)
> Change...
> 
> (249649,,now(),29,'UNCONFIRMED','NEW')''])
> 
> to read...
> 
> (249649,0,now(),29,'UNCONFIRMED','NEW')''])
> 
> in the code and that should fix it.


That's not in the code ;)

It's all generated.  Whatever's generating it is screwing something up.  Most
likely it's an undefined variable.

Comment 3

[Nick Barnes]

It's the code for "confirmation by acclamation" following a possible change to
votestoconfirm.  The backtrace indicates that $who in this code is getting
undefined. 

            SendSQL("SELECT who FROM votes WHERE bug_id = $id");
            my $who = FetchOneColumn();
            CheckIfVotedConfirmed($id, $who);

which means that there are no rows matching the select.  Reading the rest of the
code I'm not sure how this can be (unless votestoconfirm is zero?!).

Comment 4

[Nick Barnes]

Is there a reason why CheckIfVoteConfirmed does a simple select and then tests a
condition on the results, instead of putting the condition in the select?  Like
this:
    SendSQL("SELECT bugs.everconfirmed " .
            "FROM bugs, products " .
            "WHERE bugs.bug_id = $id AND products.id = bugs.product_id " .
            "AND bugs.votes >= products.votestoconfirm " .
            "AND bugs.bug_status = '$::unconfirmedstate'");
    if (MoreSQLData()) { {
        my $everconfirmed = FetchOneColumn();

instead of this:

    SendSQL("SELECT bugs.votes, bugs.bug_status, products.votestoconfirm, " .
            "       bugs.everconfirmed " .
            "FROM bugs, products " .
            "WHERE bugs.bug_id = $id AND products.id = bugs.product_id");
    my ($votes, $status, $votestoconfirm, $everconfirmed) = (FetchSQLData());
    if ($votes >= $votestoconfirm && $status eq $::unconfirmedstate) {

It wouldn't fix this problem, mind.  I think for this problem we could default
$who to zero if undefined in editproducts.cgi.  Something like this:

        foreach my $id (@list) {
            SendSQL("SELECT who FROM votes WHERE bug_id = $id");
            my $who = FetchOneColumn();
>           if (!defined($who)) {
>              # no votes for this bug
>              $who = 0;
>           }
            CheckIfVotedConfirmed($id, $who);
        }

but I'm not sure what the actual semantics should be if votestoconfirm is set to
zero and we have UNCONFIRMED bugs.  Maybe this whole part of editproducts.cgi
should be if ($votestoconfirm > 0) ?

Comment 5

[Nick Barnes]

Thinking about it, it's pretty clear that we should be passing $::userid to
CheckIfVotedConfirmed() anyway.  Getting a random voter out of the votes table
to pass here is really bogus.

Comment 6

[Nick Barnes]

Now reproduced on 2.18 branch.  As I suspected I can do this by setting
votestoconfirm to zero on a product which has an UNCONFIRMED bug.  I don't see
how it can be made to break by updating maxvotesperbug (unless it's updated to
zero, in which case the earlier code in the $checkvotes block will remove all
the votes that it has, but in that case there would be output between the
"Checking existing votes in this product..." line and the "Software error..."
line).  Anyway, still convinced that the right thing to do is to pass $::userid.
 I'll attach a patch to that effect.

Comment 7

[Nick Barnes]

A related problem, editproducts.cgi line 1241:
                  my $id = FetchSQLData();
should be:
                  my ($id) = FetchSQLData();

This bug completely stops that code from working (removing votes from a user if
votesperuser changes and the user now has too many votes in total).  While I'm
here I'll see if I can test this fairly well and submit a patch for the whole
routine.

Comment 8

[Nick Barnes]

Note that in fact the votesperuser code in editproducts.cgi can't do the right
thing except in the special case that votesperuser is reduced below the number
of votes which a user has cast for a single bug.  This is because the
RemoveVotes() routine in globals.pl does not enforce votesperuser.  See the
comment around line  1365 of globals.pl.  I'll add a comment to editproducts.cgi.

Comment 9

[Nick Barnes]

Attachment: Patch to 2.18 branch tip

OK, here's a patch to the 2.18 branch which appears to fix this bug and also
cleans up the surrounding code a little (not much; for instance I've left
CheckIfVotedConfirmed alone)

Comment 10

[Max Kanat-Alexander]

Comment on attachment 168213 [details] [diff] [review]
Patch to 2.18 branch tip

>     if ($checkvotes) {
>-        print "Checking existing votes in this product for anybody who now has too many votes.";
>+        # 1. too many votes for a single user on a single bug.
>         if ($maxvotesperbug < $votesperuser) {
>+            print "<br>Checking existing votes in this product for anybody who now has too many votes for a single bug.";

  Um, shouldn't the print statement actually stay where it is...?


>+        while (MoreSQLData()) {
>+            CheckIfVotedConfirmed(FetchOneColumn(), $::userid);
>         }

  Whoa, whoa... First off, I think we use Bugzilla->user nowadays instead of
$::userid.

  Also, this will just check the *admin's* votes, won't it?? This code doesn't
do the same thing as the old code does, which was check all the votes for each
person on each bug in the product, as I understood it.

  I don't think I have the authority yet to review- this patch, though.

Comment 11

[Vlad Dascalu]

Oh, travis discovered that it's linked from the bottom. The link at the top is
for ?help=1.

Comment 12

[Nick Barnes]

(In reply to comment #10)
> (From update of attachment 168213 [details] [diff] [review])
> >     if ($checkvotes) {
> >-        print "Checking existing votes in this product for anybody who now
has too many votes.";
> >+        # 1. too many votes for a single user on a single bug.
> >         if ($maxvotesperbug < $votesperuser) {
> >+            print "<br>Checking existing votes in this product for anybody
who now has too many votes for a single bug.";
> 
>   Um, shouldn't the print statement actually stay where it is...?

I tried to make the print output clearer.  Before there was a single print
statement:
  Checking existing votes in this product for anybody who now has too many votes.
to cover two lots of checking code.  With my patch there are two print statements:
  Checking existing votes in this product for anybody who now has too many votes
for a single bug.
  Checking existing votes in this product for anybody who now has too many total
votes.
one for each check, and the first statement is within the conditional which
controls the check.  I think this makes the output clearer and less misleading,
but YMMV.  This is the same reason why I added comments to the code.

> >+        while (MoreSQLData()) {
> >+            CheckIfVotedConfirmed(FetchOneColumn(), $::userid);
> >         }

>   Whoa, whoa... First off, I think we use Bugzilla->user nowadays instead of
> $::userid.

OK; I was basing this on other code in the same file and elsewhere (e.g. in
globals.pl).  I guess it should be Bugzilla->user->id.

> Also, this will just check the *admin's* votes, won't it?? This code doesn't
> do the same thing as the old code does, which was check all the votes for each
> person on each bug in the product, as I understood it.

No.  That's not what the second argument to CheckIfVotedConfirmed() means.  Read
the code (in CGI.pl).  The code checks the total number of bugs (i.e.
bugs.votes).  The user ID in the second argument is used for bugs_activity,
AppendComment, and the process/results.html.tmpl: it should be the user who is
causing this activity on the bug (i.e. whatever admin is running
editproducts.cgi, i.e. $::userid or Bugzilla->user).

>   I don't think I have the authority yet to review- this patch, though.

OK.  I'll submit another patch using Bugzilla->user->id and with a comment to
explain the use of it in the call to CheckIfVotedConfirmed().  I'll also mark it
review?

Comment 13

[Nick Barnes]

(In reply to comment #12)
Let me try to clarify that comment, because it didn't come out too
well.

Without the patch there is just one general print statement in this
block of code:

"Checking existing votes in this product for anybody who now has too many votes."

But the block of code is actually in three separate sections:

A. if ($maxvotesperbug < $votesperuser) {
        ... find vote records with more than $maxvotesperbug ...
   }
B. ... find users who have too many total bugs in this product ...
C. ... find unconfirmed bugs which now have enough votes to be confirmed ...

So I felt that the print statement was misleading.  In the patch I
removed it and put in three print statements to match the three
sections of code.  I also put in a comment for each section,
explaining what it does (and why section B doesn't really achieve the
thing it sets out to do).  I put the print statement for section A
inside the conditional, because if the condition isn't met then the
code doesn't get executed so it's misleading to do the print.

I think the resulting code, and its output, are clearer.

This is completely separate from fixing this bug, by the way, and
maybe should come out of the patch.  This bug is fixed by the last part of my
patch, which removes the attempt to identify a "culprit" for the bug
confirmation from the votes table, and owns up to the fact that the true culprit
is the user running editproducts.cgi.

Comment 14

[Vlad Dascalu]

Nick: is the patch for the trunk as well? :)

Comment 15

[Nick Barnes]

I'll check.  I haven't looked at trunk code for a while.

Comment 16

[Nick Barnes]

Looks as if it should apply on the trunk as well but I don't have a working
trunk installation.  I just updated but checksetup gives me this:

Can't locate Bugzilla/Auth/Verify/DB.pm in @INC (@INC contains:
/usr/local/lib/perl5/site_perl/5.6.1 . /usr/local/lib/perl5/site_perl/5.6.1/mach
/usr/local/lib/perl5/site_perl/5.005/i386-freebsd
/usr/local/lib/perl5/site_perl/5.005 /usr/local/lib/perl5/site_perl
/usr/local/lib/perl5/5.6.1/BSDPAN /usr/local/lib/perl5/5.6.1/mach
/usr/local/lib/perl5/5.6.1) at Bugzilla/Auth.pm line 41.
BEGIN failed--compilation aborted at Bugzilla/Auth.pm line 43.

I'll see if I can figure that out.

Comment 17

[Vlad Dascalu]

That's probably because you forget to get via cvs new directories. I think you
can use -P as a cvs flag in order to get those as well.

I suggest having 2 directories, one with 2.18 tip and another one with the trunk
tip :-)

Comment 18

[Nick Barnes]

Yes, my fault for expecting CVS to be not brain-dead.  Sorted now (cvs update
-d), but I have to go so I can't check this patch against the trunk until later.

I *do* have two directories.

Comment 19

[Vlad Dascalu]

Comment on attachment 168213 [details] [diff] [review]
Patch to 2.18 branch tip

>> I don't see
how it can be made to break by updating maxvotesperbug (unless it's updated to
zero, in which case the earlier code in the $checkvotes block will remove all
the votes that it has, but in that case there would be output between the
"Checking existing votes in this product..." line and the "Software error..."
line). <<

Well, you just discovered that doesn't work reliably :-) So I guess now the
mystery is fully explained.

If you have time for those comments, or for the $::userid thing, or for the tip
patch, it would be really cool. You can carry over my review on those. I guess
either you or the commiter will have to do those :)

Comment 20

[Vlad Dascalu]

Comment #11 in this bug should have been in bug 251502 comment #15 so feel free
to ignore it! :)

Comment 21

[Dave Miller [:justdave]]

(In reply to comment #13)
> This bug is fixed by the last part of my
> patch, which removes the attempt to identify a "culprit" for the bug
> confirmation from the votes table, and owns up to the fact that the true culprit
> is the user running editproducts.cgi.

Ooohhh...  does this by any chance fix bug 271577 as well?  Since the voting was
changed around in editproducts.cgi when that bug was reported, I suspect the
resulting mail from that is actually what triggered that bug.

Comment 22

[Vlad Dascalu]

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

Comment 23

[Vlad Dascalu]

Thanks Nick!

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.61; previous revision: 1.60
done

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.53.2.4; previous revision: 1.53.2.3
done

Comment 24

[Frédéric Buclin]

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

Comment 25

[Frédéric Buclin]

This patch prevents editproducts.cgi to correctly check for bugs having enough
votes to get confirmed.

This is a regression, see bug 276600.

Comment 26

[Frédéric Buclin]

(no mid-air collision about dependencies??)