302785 - [PostgreSQL] Bugzilla cannot order a buglist by number of votes

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Emmanuel Seyman]

Trying to order a buglist by number of votes gives the error message:

Software error:

DBD::Pg::st execute failed: ERREUR:  erreur de syntaxe sur ou près de «desc» at
character 919
 [for Statement "SELECT bugs.bug_id, bugs.bug_severity, bugs.priority,
bugs.bug_status, bugs.resolution, bugs.bug_severity, bugs.priority,
bugs.rep_platform, map_assigned_to.login_name, bugs.bug_status, bugs.resolution,
bugs.votes, bugs.short_desc FROM bugs  INNER JOIN profiles AS map_assigned_to ON
(bugs.assigned_to = map_assigned_to.userid) LEFT JOIN bug_group_map  ON
bug_group_map.bug_id = bugs.bug_id  AND bug_group_map.group_id NOT IN
(3,7,10,5,1,8,11,4,2,6,9)  LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who =
1 WHERE 1 = 1 AND bugs.creation_ts IS NOT NULL AND ((bug_group_map.group_id IS
NULL)    OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1)     OR
(bugs.cclist_accessible = 1 AND cc.who IS NOT NULL)     OR (bugs.assigned_to =
1) ) GROUP BY bugs.bug_id, bugs.bug_severity, bugs.priority, bugs.bug_status,
bugs.resolution, bugs.rep_platform, map_assigned_to.login_name, bugs.votes,
bugs.short_desc, bugs.votes desc, bugs.bug_id  ORDER BY bugs.votes
desc,bugs.bug_id "] at /var/www/html/bugzilla-cvs/buglist.cgi line 828


I can reproduce this on cvs tip.

Comment 1

[Frédéric Buclin]

(In reply to comment #0)

> GROUP BY bugs.bug_id, bugs.bug_severity, bugs.priority, bugs.bug_status,
> bugs.resolution, bugs.rep_platform, map_assigned_to.login_name, bugs.votes,
> bugs.short_desc, bugs.votes desc, bugs.bug_id  ORDER BY bugs.votes
> desc,bugs.bug_id "] at /var/www/html/bugzilla-cvs/buglist.cgi line 828

We have "GROUP BY bugs.votes desc" as well as "ORDER BY bugs.votes desc".
Clearly GROUP BY shouldn't have "desc" included.

manu, can you reproduce using MySQL?

Comment 2

[Max Kanat-Alexander]

This can be handled quite easily. I actually once wrote a patch to handle it,
but I thought it was unnecessary.

Now that I know it's required, I can do it quite easily.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

This patch fixes the problem for me. It affects PostgreSQL only as MySQL
doesn't care about @groupby.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 196083 [details] [diff] [review]
patch, v1

That's a complex-enough regex that I'd really like to see a comment above it
explaining what it does. :-)

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> That's a complex-enough regex that I'd really like to see a comment above it
> explaining what it does. :-)

That's not what I call a complex regexp. ;) In words, it validates strings of
the form:

(maybe there is AS)(there must be a word, or word1.word2)(maybe there is ASC or
DESC).

I don't think it worths a comment.

Comment 6

[Max Kanat-Alexander]

Just put in that line that you typed, just now. That made it clearer for me. :-)

The way I tell if code is readable or not is I look at it and say, "Is that
*instantly* clear to me?" The regexp I have to look at for 10 or more seconds to
clear it up. Imagine if I had to look at every line of code for 10 seconds...
:-) I'd never be able to get through reading anything! :-)

Comment 7

[Frédéric Buclin]

Attachment: patch, v1.5

is this comment explicit enough? ;)

Comment 8

[Max Kanat-Alexander]

Comment on attachment 196088 [details] [diff] [review]
patch, v1.5

Yes, that comment is beautiful. Thank you. :-)

Comment 9

[Joel Peshkin]

Comment on attachment 196088 [details] [diff] [review]
patch, v1.5


>+        # The structure of fields is of the form:
>+        # [foo AS] {bar | bar.baz} [ASC | DESC]
>+        # Only the mandatory part bar OR bar.baz is of interest
>+        if ($field =~ /(.*AS\s+)?(\w+(\.\w+)?)(\s+(ASC|DESC))?$/i) {
>+            push(@groupby, $2) if !grep($_ eq $2, @groupby);
>

This is pretty good, but let's also protect ourselves from confusion if a field
name ends with "as" so try....

 if ($field =~ /(?:.*\s+AS\s+)?([\w.]+)(?:\s+(ASC|DESC))?$/i) 
and then use $1

Comment 10

[Frédéric Buclin]

Attachment: patch, v2

I prefer the more restrictive \w+(.\w+)? regexp than [\w.]+ which would accept
'.foo.' for instance.

Comment 11

[Dave Miller [:justdave]]

holding for 2.20.1

Comment 12

[Frédéric Buclin]

tip:

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.114; previous revision: 1.113
done

2.20:

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.99.2.6; previous revision: 1.99.2.5
done