155584 - Opening duplicates.cgi with no frequent bugs causes SQL syntax error

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Jussi Sirpoma]

Opening duplicates.cgi in an installation where no bugs are within the frequent
bug threshold causes the following SQL syntax error:

Software error:

  SELECT bugs.bug_id, component, bug_severity, op_sys, target_milestone,
         short_desc, bug_status, resolution
  FROM bugs 
  LEFT JOIN cc selectVisible_cc ON 
                     bugs.bug_id = selectVisible_cc.bug_id AND 
                     selectVisible_cc.who = 1 WHERE ((bugs.groupset &
9223372036854775807) = bugs.groupset OR (bugs.reporter_accessible = 1 AND
bugs.reporter = 1) OR (bugs.cclist_accessible = 1 AND selectVisible_cc.who = 1
AND not isnull(selectVisible_cc.who)) OR (bugs.assigned_to = 1) OR
(bugs.qa_contact = 1)) AND (bug_status != 'CLOSED') 
  AND   ((bug_status = 'VERIFIED' AND resolution IN ('INVALID', 'WONTFIX')) 
         OR (bug_status != 'VERIFIED'))
  AND bugs.bug_id IN (): You have an error in your SQL syntax near ')' at line
10 at globals.pl line 276.

This bug might have been caused by the patch to bug 151281.

Comment 1

[Gervase Markham [:gerv]]

This should be 2.16. It's a regression.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Is this a reasonable way of fixing it?

Gerv

Comment 3

[Jouni Heikniemi]

Comment on attachment 90833 [details] [diff] [review]
Patch v.1

This hack is simply horrible and I hate it, but it's not a good idea to be too
picky this close to 2.16 release. :-)

>+# We add a -1 to the bug numbers so we don't get an SQL error if there
>+# are no bugs in %count.

The first time I read this, I thought you were doing a "bug number = bug number
- 1"... How about "to the list of bug numbers being searched"?

2xr=jouni anyway.

Comment 4

[Gervase Markham [:gerv]]

How would you fix it on the trunk?

Gerv

Comment 5

[Gervase Markham [:gerv]]

The duplicates.cgi performance patch, which caused this problem, didn't get
checked into 2.16. False alarm :-) So how do we fix this on the trunk?

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

if (scalar(keys(%count)) == 0) {
  DisplayError(....)
}

would work,wouldn't it?

Comment 7

[Jouni Heikniemi]

Comment on attachment 90833 [details] [diff] [review]
Patch v.1

Immediately after reviewing this I went to sleep. As soon as my head hit the
pillow, I realized that this patch wouldn't work. (-1, ) is no more correct SQL
than ().

That's what you get when you work tired. :-( I usually test even the most
trivial patches. Should've done this time too... So, revoking reviews and
marking needs-work. I'll look at this again in a couple of hours. :-)

Comment 8

[Jouni Heikniemi]

>if (scalar(keys(%count)) == 0) {
>  DisplayError(....)
>}

Yes, and actually you don't need to filter the keys either, so "scalar(%count)"
is just as fine. And maybe ThrowUserError instead of DisplayError, or do you
intend to show something else on the page as well?

Comment 9

[Gervase Markham [:gerv]]

I don't think we should be ThrowingErrors - it's a perfectly valid condition
which isn't the user's fault or a programming bug. Instead, we should be
displaying some "Not enough duplicates" message or something else similarly
sane. But that's a matter for the template to deal with when it gets given an
empty list.

Alternative fix:

$count{'-1'} = "foo";
before we do join (", ", keys %count);

Gerv

Comment 10

[Gervase Markham [:gerv]]

jouni: ping? :-)

Gerv

Comment 11

[Jouni Heikniemi]

>Alternative fix:
>$count{'-1'} = "foo";
>before we do join (", ", keys %count);

Ugh, that's _so_ ugly. :-) 

Now that I think of it, I think we should put the SQL construction and SendSQL
calls into a |if (scalar(%count))| block and make the template have a special
handling (print out something like "No duplicate bugs found.") for the situation
with a zero-length @bugs (and @bug_ids). That way we wouldn't have to put yet
another string into the cgi. 

How does this sound?

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.2

OK. This patch is somewhat bigger than the last one - but, trust me, it's just
an indenting thing. I'll attach a diff -uw.

Gerv

Comment 13

[Gervase Markham [:gerv]]

Attachment: Patch v.2w

diff -uw (remove whitespace-only changes.)

Gerv

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 94753 [details] [diff] [review]
Patch v.2w

What we really need is subselects.
The code is fine, but remember that scalar(%count) can be non-zero even though
no bugs are found (because of non-visiable bugs)

r=bbaetz x2

Comment 15

[Gervase Markham [:gerv]]

Fixed.

Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in template/en/default/reports/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates.html.tmpl,v
 <--  duplicates.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/reports/duplicates-table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/duplicates-table.html.tmpl,v
 <--  duplicates-table.html.tmpl
new revision: 1.5; previous revision: 1.4
done

Gerv