314454 - Incorrect SQL query in editproducts.cgi when making a group mandatory

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

This is a regression due to bug 289043. In editproducts.cgi at line 511:

            $mandatory_groups = $dbh->selectall_arrayref(
                    'SELECT groups.name, COUNT(bugs.bug_id) AS count
                       FROM bugs
                  LEFT JOIN bug_group_map
                         ON bug_group_map.bug_id = bugs.bug_id
                 INNER JOIN groups
                         ON bug_group_map.group_id = groups.id
                      WHERE groups.id IN (' . join(', ', @now_mandatory) . ')
                        AND bugs.product_id = ?
                        AND bug_group_map.bug_id IS NULL ' .
                       $dbh->sql_group_by('groups.name'),
                   {'Slice' => {}}, $product->id);

This request returns no result because 'INNER JOIN groups' is called after 'LEFT JOIN bug_group_map'.

Comment 1

[Elliotte Martin]

Attachment: v1

Used Inner Join in query instead of Outer to get it to return records.

Comment 2

[Max Kanat-Alexander]

Could somebody (LpSolit?) explain what the user-facing behavior is supposed to be and what it is instead? I can hardly understand this code. :-)

Comment 3

[Elliotte Martin]

With the corrected SQL, a confirmation prompt will now come up if a MemberControl for any group is changed to Mandatory.  The confirmation will also come up if a MemberControl is set to NA, but the bug was that changing a MemberControl to Mandatory (and no MemberControls were set to NA) then the confirmation would be bypassed.

Comment 4

[Frédéric Buclin]

(In reply to comment #2)
> Could somebody (LpSolit?) explain what the user-facing behavior is supposed to
> be and what it is instead? I can hardly understand this code. :-)

No worry, I will review it. :)

Comment 5

[Frédéric Buclin]

Comment on attachment 311972 [details] [diff] [review]
v1

>-                  LEFT JOIN bug_group_map
>+                 INNER JOIN bug_group_map

>-                        AND bug_group_map.bug_id IS NULL ' .
>+                        AND bugs.product_id = ? ' .

This doesn't work. We want bugs with no entry in the bug_group_map table. What you do is exactly the opposite.

Comment 6

[Elliotte Martin]

Attachment: v2

There's probably a more elegant way to do this, but this will get the bugs with no entry in the bug_group_map table for the mandatory groups.

Comment 7

[Frédéric Buclin]

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

I cannot believe something as simple as this must require 5 consecutive SELECT. Must be doable with a NOT EXISTS or an UNION. I don't have the right idea yet, but this may come when I have time to investigate this a bit more.

Why not look for all bugs in the given product_id and which are not in the bug_group_map table AND do an UNION with bugs which are in the bug_group_map table but not with the correct group(s)? This should make the query much simple.

Comment 8

[Max Kanat-Alexander]

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

If this works, doing it with subselects is fine by me.

Do capitalize SELECT, though.

LpSolit is right that you can do what you want with EXISTS instead of the > 0, which would probably be better.

I'd like the formatting to be a bit different, also--it's very difficult to determine where that first subselect ends.

Comment 9

[Elliotte Martin]

Attachment: v3

I used EXISTS instead of > 0 to clean up the query, otherwise I couldn't find a simpler way of doing the query.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 313035 [details] [diff] [review]
v3

Yes, this code looks great, thanks for the formatting fix, that makes it way easier to read.

I'm fine with the code, LpSolit--could you tell me if this does what it's supposed to do? :-)

>+                           (SELECT COUNT(bugs.bug_id)
>+                              FROM bugs
>+                             WHERE bugs.product_id = ?
>+                               AND bugs.bug_id NOT IN
>+                           (SELECT bug_group_map.bug_id FROM bug_group_map
>+                             WHERE bug_group_map.group_id = groups.id))

  One minor nit is that that second SELECT there should probably be indented to note that it's part of the NOT IN, not part of the main query there.

>Index: index.cgi

  And I don't think this change needs to be checked in, at least not as part of this patch, yes?

Comment 11

[Elliotte Martin]

(In reply to comment #10)
> (From update of attachment 313035 [details] [diff] [review])
> Yes, this code looks great, thanks for the formatting fix, that makes it way
> easier to read.
> I'm fine with the code, LpSolit--could you tell me if this does what it's
> supposed to do? :-)
> >+                           (SELECT COUNT(bugs.bug_id)
> >+                              FROM bugs
> >+                             WHERE bugs.product_id = ?
> >+                               AND bugs.bug_id NOT IN
> >+                           (SELECT bug_group_map.bug_id FROM bug_group_map
> >+                             WHERE bug_group_map.group_id = groups.id))
>   One minor nit is that that second SELECT there should probably be indented to
> note that it's part of the NOT IN, not part of the main query there.
> >Index: index.cgi
>   And I don't think this change needs to be checked in, at least not as part of
> this patch, yes?

Sorry, the index.cgi change should not have been in there.

Comment 12

[Frédéric Buclin]

Comment on attachment 313035 [details] [diff] [review]
v3

>         if (@now_mandatory) {
>             $mandatory_groups = $dbh->selectall_arrayref(
>+                    'SELECT groups.name,
>+                           (SELECT COUNT(bugs.bug_id)
>+                              FROM bugs
>+                             WHERE bugs.product_id = ?
>+                               AND bugs.bug_id NOT IN
>+                           (SELECT bug_group_map.bug_id FROM bug_group_map
>+                             WHERE bug_group_map.group_id = groups.id))
>+                           AS count
>+                      FROM groups
>+                        WHERE groups.id IN (' . join(', ', @now_mandatory) . ')
>+                           AND EXISTS (SELECT bugs.bug_id FROM bugs
>+                                        WHERE bugs.product_id = ?
>+                                          AND bugs.bug_id NOT IN
>+                                      (SELECT bug_group_map.bug_id FROM bug_group_map
>+                                        WHERE bug_group_map.group_id = groups.id))
>+                        ORDER BY groups.name',

It's irritating that PostgreSQL requires to rewrite the whole sub-select instead of having
  WHERE groups.id IN (...) AND count > 0

For clarity, maybe could we drop the AND EXISTS () block completely and remove zero counts ourselves with:
  @$mandatory_groups = grep { $_->{count} } @$mandatory_groups;

Max, what do you prefer?

Comment 13

[Frédéric Buclin]

Holding approval till Max's answer.

Comment 14

[Max Kanat-Alexander]

Hm, yeah, actually, for simplicity's sake I think your idea about the grep is a good one.

Comment 15

[Frédéric Buclin]

Elliotte, do you want to update the patch yourself, or do you prefer us to do it?

Comment 16

[Elliotte Martin]

(In reply to comment #15)
> Elliotte, do you want to update the patch yourself, or do you prefer us to do
> it?

I'll do it.  I was tied up with a conference for a few days and couldn't get to it.

Comment 17

[Elliotte Martin]

Attachment: v4

Simplifies SQL query by adding grep to remove groups with a count of zero.

Comment 18

[Frédéric Buclin]

Comment on attachment 315194 [details] [diff] [review]
v4

Works fine on both trunk and 3.0. r=LpSolit

Comment 19

[Frédéric Buclin]

tip:

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


3.0.4:

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