Bug 491467: Make Search.pm and buglist.cgi consistently take column ids for the "fields" and "order" arguments, to prevent problems with using SQL fragments in the order and columnlist.

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=wicked, a=mkanat

9 files changed, 304 insertions, 294 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index f7d00f3b3..4bf064375 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -52,7 +52,6 @@ use Storable qw(dclone);
 
 use constant BLOB_TYPE => DBI::SQL_BLOB;
 use constant ISOLATION_LEVEL => 'REPEATABLE READ';
-use constant GROUPBY_REGEXP => '(?:.*\s+AS\s+|SUBSTRING\()?(\w+(\.\w+)?)(?:\s+(ASC|DESC|FROM\s+.+))?$';
 
 # Set default values for what used to be the enum types.  These values
 # are no longer stored in localconfig.  If we are upgrading from a
diff --git a/Bugzilla/DB/Oracle.pm b/Bugzilla/DB/Oracle.pm
index 23b97a0d2..a2c78e094 100644
--- a/Bugzilla/DB/Oracle.pm
+++ b/Bugzilla/DB/Oracle.pm
@@ -52,7 +52,6 @@ use base qw(Bugzilla::DB);
 use constant EMPTY_STRING  => '__BZ_EMPTY_STR__';
 use constant ISOLATION_LEVEL => 'READ COMMITTED';
 use constant BLOB_TYPE => { ora_type => ORA_BLOB };
-use constant GROUPBY_REGEXP => '((CASE\s+WHEN.+END)|(SUBSTR.+\))|(TO_CHAR\(.+\))|(\(SCORE.+\))|(\(MATCH.+\))|(\w+(\.\w+)?))(\s+AS\s+)?(.*)?$';
 
 sub new {
     my ($class, $user, $pass, $host, $dbname, $port) = @_;
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 0c0a76562..c606b774d 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -33,7 +33,13 @@ use strict;
 
 package Bugzilla::Search;
 use base qw(Exporter);
-@Bugzilla::Search::EXPORT = qw(IsValidQueryType);
+@Bugzilla::Search::EXPORT = qw(
+    EMPTY_COLUMN
+
+    IsValidQueryType
+    split_order_term
+    translate_old_column
+);
 
 use Bugzilla::Error;
 use Bugzilla::Util;
@@ -47,21 +53,126 @@ use Bugzilla::Keyword;
 use Date::Format;
 use Date::Parse;
 
+# A SELECTed expression that we use as a placeholder if somebody selects
+# <none> for the X, Y, or Z axis in report.cgi.
+use constant EMPTY_COLUMN => '-1';
+
 # Some fields are not sorted on themselves, but on other fields. 
 # We need to have a list of these fields and what they map to.
 # Each field points to an array that contains the fields mapped 
 # to, in order.
 use constant SPECIAL_ORDER => {
-    'bugs.target_milestone' => [ 'ms_order.sortkey','ms_order.value' ],
+    'target_milestone' => [ 'ms_order.sortkey','ms_order.value' ],
 };
 
 # When we add certain fields to the ORDER BY, we need to then add a
 # table join to the FROM statement. This hash maps input fields to 
 # the join statements that need to be added.
 use constant SPECIAL_ORDER_JOIN => {
-    'bugs.target_milestone' => 'LEFT JOIN milestones AS ms_order ON ms_order.value = bugs.target_milestone AND ms_order.product_id = bugs.product_id',
+    'target_milestone' => 'LEFT JOIN milestones AS ms_order ON ms_order.value = bugs.target_milestone AND ms_order.product_id = bugs.product_id',
 };
 
+# This constant defines the columns that can be selected in a query 
+# and/or displayed in a bug list.  Column records include the following
+# fields:
+#
+# 1. id: a unique identifier by which the column is referred in code;
+#
+# 2. name: The name of the column in the database (may also be an expression
+#          that returns the value of the column);
+#
+# 3. title: The title of the column as displayed to users.
+# 
+# Note: There are a few hacks in the code that deviate from these definitions.
+#       In particular, when the list is sorted by the "votes" field the word 
+#       "DESC" is added to the end of the field to sort in descending order, 
+#       and the redundant short_desc column is removed when the client
+#       requests "all" columns.
+#
+# This is really a constant--that is, once it's been called once, the value
+# will always be the same unless somebody adds a new custom field. But
+# we have to do a lot of work inside the subroutine to get the data,
+# and we don't want it to happen at compile time, so we have it as a
+# subroutine.
+sub COLUMNS {
+    my $dbh = Bugzilla->dbh;
+    my $cache = Bugzilla->request_cache;
+    return $cache->{search_columns} if defined $cache->{search_columns};
+
+    # These are columns that don't exist in fielddefs, but are valid buglist
+    # columns. (Also see near the bottom of this function for the definition
+    # of short_short_desc.)
+    my %columns = (
+        relevance            => { title => 'Relevance'  },
+        assigned_to_realname => { title => 'Assignee'   },
+        reporter_realname    => { title => 'Reporter'   },
+        qa_contact_realname  => { title => 'QA Contact' },
+    );
+
+    # Next we define columns that have special SQL instead of just something
+    # like "bugs.bug_id".
+    my $actual_time = '(SUM(ldtime.work_time)'
+        . ' * COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id))';
+    my %special_sql = (
+        deadline    => $dbh->sql_date_format('bugs.deadline', '%Y-%m-%d'),
+        actual_time => $actual_time,
+
+        percentage_complete =>
+            "(CASE WHEN $actual_time + bugs.remaining_time = 0.0"
+              . " THEN 0.0"
+              . " ELSE 100"
+                   . " * ($actual_time / ($actual_time + bugs.remaining_time))"
+              . " END)",
+    );
+
+    # Backward-compatibility for old field names. Goes new_name => old_name.
+    # These are here and not in translate_old_column because the rest of the
+    # code actually still uses the old names, while the fielddefs table uses
+    # the new names (which is not the case for the fields handled by 
+    # translate_old_column).
+    my %old_names = (
+        creation_ts => 'opendate',
+        delta_ts    => 'changeddate',
+        work_time   => 'actual_time',
+    );
+
+    # Fields that are email addresses
+    my @email_fields = qw(assigned_to reporter qa_contact);
+    # Other fields that are stored in the bugs table as an id, but
+    # should be displayed using their name.
+    my @id_fields = qw(product component classification);
+
+    foreach my $col (@email_fields) {
+        my $sql = "map_${col}.login_name";
+        if (!Bugzilla->user->id) {
+             $sql = $dbh->sql_string_until($sql, $dbh->quote('@'));
+        }
+        $special_sql{$col} = $sql;
+        $columns{"${col}_realname"}->{name} = "map_${col}.realname";
+    }
+
+    foreach my $col (@id_fields) {
+        $special_sql{$col} = "map_${col}s.name";
+    }
+
+    # Do the actual column-getting from fielddefs, now.
+    foreach my $field (Bugzilla->get_fields({ obsolete => 0, buglist => 1 })) {
+        my $id = $field->name;
+        $id = $old_names{$id} if exists $old_names{$id};
+        my $sql = 'bugs.' . $field->name;
+        $sql = $special_sql{$id} if exists $special_sql{$id};
+        $columns{$id} = { name => $sql, title => $field->description };
+    }
+
+    # The short_short_desc column is identical to short_desc
+    $columns{'short_short_desc'} = $columns{'short_desc'};
+
+    Bugzilla::Hook::process("buglist-columns", { columns => \%columns });
+
+    $cache->{search_columns} = \%columns;
+    return $cache->{search_columns};
+}
+
 # Create a new Search
 # Note that the param argument may be modified by Bugzilla::Search
 sub new {
@@ -78,22 +189,18 @@ sub new {
 
 sub init {
     my $self = shift;
-    my $fieldsref = $self->{'fields'};
+    my @fields = @{ $self->{'fields'} || [] };
     my $params = $self->{'params'};
     $self->{'user'} ||= Bugzilla->user;
     my $user = $self->{'user'};
 
-    my $orderref = $self->{'order'} || 0;
-    my @inputorder;
-    @inputorder = @$orderref if $orderref;
+    my @inputorder = @{ $self->{'order'} || [] };
     my @orderby;
 
-    my @fields;
     my @supptables;
     my @wherepart;
     my @having;
     my @groupby;
-    @fields = @$fieldsref if $fieldsref;
     my @specialchart;
     my @andlist;
     my %chartfields;
@@ -109,48 +216,56 @@ sub init {
         obsolete => 0 });
     foreach my $field (@select_fields) {
         my $name = $field->name;
-        $special_order{"bugs.$name"} = [ "$name.sortkey", "$name.value" ],
-        $special_order_join{"bugs.$name"} =
-            "LEFT JOIN $name ON $name.value = bugs.$name";
+        next if $name eq 'product'; # products don't have sortkeys.
+        $special_order{$name} = [ "$name.sortkey", "$name.value" ],
+        $special_order_join{$name} =
+           "LEFT JOIN $name ON $name.value = bugs.$name";
     }
 
     my $dbh = Bugzilla->dbh;
-
+   
+    # All items that are in the ORDER BY must be in the SELECT.
+    foreach my $orderitem (@inputorder) {
+        my $column_name = split_order_term($orderitem);
+        if (!grep($_ eq $column_name, @fields)) {
+            push(@fields, $column_name);
+        }
+    }
+ 
     # First, deal with all the old hard-coded non-chart-based poop.
-    if (grep(/map_assigned_to/, @$fieldsref)) {
+    if (grep(/^assigned_to/, @fields)) {
         push @supptables, "INNER JOIN profiles AS map_assigned_to " .
                           "ON bugs.assigned_to = map_assigned_to.userid";
     }
 
-    if (grep(/map_reporter/, @$fieldsref)) {
+    if (grep(/^reporter/, @fields)) {
         push @supptables, "INNER JOIN profiles AS map_reporter " .
                           "ON bugs.reporter = map_reporter.userid";
     }
 
-    if (grep(/map_qa_contact/, @$fieldsref)) {
+    if (grep(/^qa_contact/, @fields)) {
         push @supptables, "LEFT JOIN profiles AS map_qa_contact " .
                           "ON bugs.qa_contact = map_qa_contact.userid";
     }
 
-    if (lsearch($fieldsref, 'map_products.name') >= 0) {
+    if (grep($_ eq 'product' || $_ eq 'classification', @fields)) 
+    {
         push @supptables, "INNER JOIN products AS map_products " .
                           "ON bugs.product_id = map_products.id";
     }
 
-    if (lsearch($fieldsref, 'map_classifications.name') >= 0) {
-        push @supptables, "INNER JOIN products AS map_products " .
-                          "ON bugs.product_id = map_products.id";
+    if (grep($_ eq 'classification', @fields)) {
         push @supptables,
                 "INNER JOIN classifications AS map_classifications " .
                 "ON map_products.classification_id = map_classifications.id";
     }
 
-    if (lsearch($fieldsref, 'map_components.name') >= 0) {
+    if (grep($_ eq 'component', @fields)) {
         push @supptables, "INNER JOIN components AS map_components " .
                           "ON bugs.component_id = map_components.id";
     }
     
-    if (grep($_ =~/AS (actual_time|percentage_complete)$/, @$fieldsref)) {
+    if (grep($_ eq 'actual_time' || $_ eq 'percentage_complete', @fields)) {
         push(@supptables, "LEFT JOIN longdescs AS ldtime " .
                           "ON ldtime.bug_id = bugs.bug_id");
     }
@@ -758,13 +873,6 @@ sub init {
     # to other parts of the query, so we want to create it before we
     # write the FROM clause.
     foreach my $orderitem (@inputorder) {
-        # Some fields have 'AS' aliases. The aliases go in the ORDER BY,
-        # not the whole fields.
-        # XXX - Ideally, we would get just the aliases in @inputorder,
-        # and we'd never have to deal with this.
-        if ($orderitem =~ /\s+AS\s+(.+)$/i) {
-            $orderitem = $1;
-        }
         BuildOrderBy(\%special_order, $orderitem, \@orderby);
     }
     # Now JOIN the correct tables in the FROM clause.
@@ -772,9 +880,9 @@ sub init {
     # cleaner to do it this way.
     foreach my $orderitem (@inputorder) {
         # Grab the part without ASC or DESC.
-        my @splitfield = split(/\s+/, $orderitem);
-        if ($special_order_join{$splitfield[0]}) {
-            push(@supptables, $special_order_join{$splitfield[0]});
+        my $column_name = split_order_term($orderitem);
+        if ($special_order_join{$column_name}) {
+            push(@supptables, $special_order_join{$column_name});
         }
     }
 
@@ -803,7 +911,9 @@ sub init {
     # Make sure we create a legal SQL query.
     @andlist = ("1 = 1") if !@andlist;
 
-    my $query = "SELECT " . join(', ', @fields) .
+    my @sql_fields = map { $_ eq EMPTY_COLUMN ? EMPTY_COLUMN 
+                           : COLUMNS->{$_}->{name} . ' AS ' . $_ } @fields;
+    my $query = "SELECT " . join(', ', @sql_fields) .
                 " FROM $suppstring" .
                 " LEFT JOIN bug_group_map " .
                 " ON bug_group_map.bug_id = bugs.bug_id ";
@@ -830,17 +940,21 @@ sub init {
         }
     }
 
-    foreach my $field (@fields, @orderby) {
-        next if ($field =~ /(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i ||
-                 $field =~ /^\d+$/ || $field eq "bugs.bug_id" ||
-                 $field =~ /^(relevance|actual_time|percentage_complete)/);
-        # 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.
-        # But for Oracle, it needs the real name part instead.
-        my $regexp = $dbh->GROUPBY_REGEXP;
-        if ($field =~ /$regexp/i) {
-            push(@groupby, $1) if !grep($_ eq $1, @groupby);
+    # For some DBs, every field in the SELECT must be in the GROUP BY.
+    foreach my $field (@fields) {
+        # These fields never go into the GROUP BY (bug_id goes in
+        # explicitly, below).
+        next if (grep($_ eq $field, EMPTY_COLUMN, 
+                      qw(bug_id actual_time percentage_complete)));
+        my $col = COLUMNS->{$field}->{name};
+        push(@groupby, $col) if !grep($_ eq $col, @groupby);
+    }
+    # And all items from ORDER BY must be in the GROUP BY. The above loop 
+    # doesn't catch items that were put into the ORDER BY from SPECIAL_ORDER.
+    foreach my $item (@inputorder) {
+        my $column_name = split_order_term($item);
+        if ($special_order{$column_name}) {
+            push(@groupby, @{ $special_order{$column_name} });
         }
     }
     $query .= ") " . $dbh->sql_group_by("bugs.bug_id", join(', ', @groupby));
@@ -1023,9 +1137,7 @@ sub IsValidQueryType
 sub BuildOrderBy {
     my ($special_order, $orderitem, $stringlist, $reverseorder) = (@_);
 
-    my @twopart = split(/\s+/, $orderitem);
-    my $orderfield = $twopart[0];
-    my $orderdirection = $twopart[1] || "";
+    my ($orderfield, $orderdirection) = split_order_term($orderitem);
 
     if ($reverseorder) {
         # If orderdirection is empty or ASC...
@@ -1052,6 +1164,40 @@ sub BuildOrderBy {
     push(@$stringlist, trim($orderfield . ' ' . $orderdirection));
 }
 
+# Splits out "asc|desc" from a sort order item.
+sub split_order_term {
+    my $fragment = shift;
+    $fragment =~ /^(.+?)(?:\s+(ASC|DESC))?$/i;
+    my ($column_name, $direction) = (lc($1), uc($2));
+    $direction ||= "";
+    return wantarray ? ($column_name, $direction) : $column_name;
+}
+
+# Used to translate old SQL fragments from buglist.cgi's "order" argument
+# into our modern field IDs.
+sub translate_old_column {
+    my ($column) = @_;
+    # All old SQL fragments have a period in them somewhere.
+    return $column if $column !~ /\./;
+
+    if ($column =~ /\bAS\s+(\w+)$/i) {
+        return $1;
+    }
+    # product, component, classification, assigned_to, qa_contact, reporter
+    elsif ($column =~ /map_(\w+?)s?\.(login_)?name/i) {
+        return $1;
+    }
+    
+    # If it doesn't match the regexps above, check to see if the old 
+    # SQL fragment matches the SQL of an existing column
+    foreach my $key (%{ COLUMNS() }) {
+        next unless exists COLUMNS->{$key}->{name};
+        return $key if COLUMNS->{$key}->{name} eq $column;
+    }
+
+    return $column;
+}
+
 #####################################################################
 # Search Functions
 #####################################################################
@@ -1276,9 +1422,9 @@ sub _content_matches {
                        "ON bugs.bug_id = $table.bug_id");
     
     # Create search terms to add to the SELECT and WHERE clauses.
-    my ($term1, $rterm1) = $dbh->sql_fulltext_search("$table.$comments_col", 
+    my ($term1, $rterm1) = $dbh->sql_fulltext_search("$table.$comments_col",
                                                         $$v, 1);
-    my ($term2, $rterm2) = $dbh->sql_fulltext_search("$table.short_desc", 
+    my ($term2, $rterm2) = $dbh->sql_fulltext_search("$table.short_desc",
                                                         $$v, 2);
     $rterm1 = $term1 if !$rterm1;
     $rterm2 = $term2 if !$rterm2;
@@ -1287,20 +1433,18 @@ sub _content_matches {
     $$term = "$term1 > 0 OR $term2 > 0";
     
     # In order to sort by relevance (in case the user requests it),
-    # we SELECT the relevance value and give it an alias so we can
-    # add it to the SORT BY clause when we build it in buglist.cgi.
-    my $select_term = "($rterm1 + $rterm2) AS relevance";
-
-    # Users can specify to display the relevance field, in which case
-    # it'll show up in the list of fields being selected, and we need
-    # to replace that occurrence with our select term.  Otherwise
-    # we can just add the term to the list of fields being selected.
-    if (grep($_ eq "relevance", @$fields)) {
-        @$fields = map($_ eq "relevance" ? $select_term : $_ , @$fields);
-    }
-    else {
-        push(@$fields, $select_term);
-    }
+    # we SELECT the relevance value so we can add it to the ORDER BY
+    # clause. Every time a new fulltext chart isadded, this adds more 
+    # terms to the relevance sql. (That doesn't make sense in
+    # "NOT" charts, but Bugzilla never uses those with fulltext
+    # by default.)
+    #
+    # We build the relevance SQL by modifying the COLUMNS list directly,
+    # which is kind of a hack but works.
+    my $current = COLUMNS->{'relevance'}->{name};
+    $current = $current ? "$current + " : '';
+    my $select_term = "($current$rterm1 + $rterm2)";
+    COLUMNS->{'relevance'}->{name} = $select_term;
 }
 
 sub _timestamp_compare {
@@ -1459,8 +1603,8 @@ sub _percentage_complete {
     }
     if ($oper ne "noop") {
         my $table = "longdescs_$$chartid";
-        if(lsearch($fields, "bugs.remaining_time") == -1) {
-            push(@$fields, "bugs.remaining_time");                  
+        if (!grep($_ eq 'remaining_time', @$fields)) {
+            push(@$fields, "remaining_time");
         }
         push(@$supptables, "LEFT JOIN longdescs AS $table " .
                            "ON $table.bug_id = bugs.bug_id");
diff --git a/buglist.cgi b/buglist.cgi
index b4a681321..5ed14ec41 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -641,92 +641,7 @@ if (!$params->param('query_format')) {
 # Column Definition
 ################################################################################
 
-# Define the columns that can be selected in a query and/or displayed in a bug
-# list.  Column records include the following fields:
-#
-# 1. ID: a unique identifier by which the column is referred in code;
-#
-# 2. Name: The name of the column in the database (may also be an expression
-#          that returns the value of the column);
-#
-# 3. Title: The title of the column as displayed to users.
-# 
-# Note: There are a few hacks in the code that deviate from these definitions.
-#       In particular, when the list is sorted by the "votes" field the word 
-#       "DESC" is added to the end of the field to sort in descending order, 
-#       and the redundant short_desc column is removed when the client
-#       requests "all" columns.
-# Note: For column names using aliasing (SQL "<field> AS <alias>"), the column
-#       ID needs to be identical to the field ID for list ordering to work.
-
-my $columns = { relevance => {name => 'relevance', title => 'Relevance'},
-                short_short_desc => {name => 'bugs.short_desc', title => 'Summary'} };
-
-foreach my $field (Bugzilla->get_fields({ obsolete => 0, buglist => 1 })) {
-    # Rename some field names for backward compatibility
-    my $id = $field->name;
-    if ($id eq 'creation_ts') {
-        $id = 'opendate';
-    }
-    elsif ($id eq 'delta_ts') {
-        $id = 'changeddate';
-    }
-    elsif ($id eq 'work_time') {
-        $id = 'actual_time';
-    }
-
-    # Database column names and expressions
-    # XXX Move these to fielddefs/Field.pm or Search.pm?
-    my $name = 'bugs.' . $field->name;
-    if ($id eq 'assigned_to' || $id eq 'reporter' || $id eq 'qa_contact') {
-        $name = 'map_' . $field->name . '.login_name';
-        if (!Bugzilla->user->id) {
-            $name = $dbh->sql_string_until($name, $dbh->quote('@'));
-        }
-    }
-    elsif ($id eq 'product' || $id eq 'component' || $id eq 'classification') {
-        $name = 'map_' . $field->name . 's.name';
-    }
-    elsif ($id eq 'deadline') {
-        $name = $dbh->sql_date_format('bugs.deadline', '%Y-%m-%d') . " AS deadline";
-    }
-    elsif ($id eq 'actual_time') {
-        $name = '(SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) AS actual_time';
-    }
-    elsif ($id eq 'percentage_complete') {
-        $name = 
-    "(CASE WHEN (SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) " .
-    "            + bugs.remaining_time = 0.0 " .
-    "THEN 0.0 " .
-    "ELSE 100*((SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) " .
-    "     /((SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) + bugs.remaining_time)) " .
-    "END) AS percentage_complete"
-    }
-
-    $columns->{$id} = { 'name' => $name, 'title' => $field->description };
-}
-
-foreach my $col (qw(assigned_to reporter qa_contact)) {
-    my $colname = "${col}_realname";
-    if ($format->{'extension'} eq 'html') {
-        my $login = "map_${col}.login_name";
-        if (!Bugzilla->user->id) {
-            $login = $dbh->sql_string_until($login, $dbh->quote('@'));
-        }
-        $columns->{$colname}->{name} =
-            "CASE WHEN map_${col}.realname = '' 
-                  THEN $login ELSE map_${col}.realname 
-                   END AS $colname";
-    }
-    else {
-        $columns->{$colname}->{name} = "map_${col}.realname AS $colname";
-    }
-}
-$columns->{assigned_to_realname}->{title} = "Assignee";
-$columns->{reporter_realname}->{title} = "Reporter";
-$columns->{qa_contact_realname}->{title} = "QA Contact";
-
-Bugzilla::Hook::process("buglist-columns", {'columns' => $columns} );
+my $columns = Bugzilla::Search::COLUMNS;
 
 ################################################################################
 # Display Column Determination
@@ -824,6 +739,18 @@ if (lsearch(\@displaycolumns, "percentage_complete") >= 0) {
     push (@selectcolumns, "actual_time");
 }
 
+# Make sure that the login_name version of a field is always also
+# requested if the realname version is requested, so that we can
+# display the login name when the realname is empty.
+my @realname_fields = grep(/_realname$/, @displaycolumns);
+foreach my $item (@realname_fields) {
+    my $login_field = $item;
+    $login_field =~ s/_realname$//;
+    if (!grep($_ eq $login_field, @selectcolumns)) {
+        push(@selectcolumns, $login_field);
+    }
+}
+
 # Display columns are selected because otherwise we could not display them.
 push (@selectcolumns, @displaycolumns);
 
@@ -865,17 +792,6 @@ if ($format->{'extension'} eq 'atom') {
 }
 
 ################################################################################
-# Query Generation
-################################################################################
-
-# Convert the list of columns being selected into a list of column names.
-my @selectnames = map($columns->{$_}->{'name'}, @selectcolumns);
-
-# Remove columns with no names, such as percentage_complete
-#  (or a removed *_time column due to permissions)
-@selectnames = grep($_ ne '', @selectnames);
-
-################################################################################
 # Sort Order Determination
 ################################################################################
 
@@ -898,39 +814,47 @@ if (!$order || $order =~ /^reuse/i) {
     }
 }
 
-my $db_order = "";  # Modified version of $order for use with SQL query
 if ($order) {
     # Convert the value of the "order" form field into a list of columns
     # by which to sort the results.
     ORDER: for ($order) {
         /^Bug Number$/ && do {
-            $order = "bugs.bug_id";
+            $order = "bug_id";
             last ORDER;
         };
         /^Importance$/ && do {
-            $order = "bugs.priority, bugs.bug_severity";
+            $order = "priority,bug_severity";
             last ORDER;
         };
         /^Assignee$/ && do {
-            $order = "map_assigned_to.login_name, bugs.bug_status, bugs.priority, bugs.bug_id";
+            $order = "assigned_to,bug_status,priority,bug_id";
             last ORDER;
         };
         /^Last Changed$/ && do {
-            $order = "bugs.delta_ts, bugs.bug_status, bugs.priority, map_assigned_to.login_name, bugs.bug_id";
+            $order = "changeddate,bug_status,priority,assigned_to,bug_id";
             last ORDER;
         };
         do {
             my @order;
-            my @columnnames = map($columns->{lc($_)}->{'name'}, keys(%$columns));
             # A custom list of columns.  Make sure each column is valid.
             foreach my $fragment (split(/,/, $order)) {
                 $fragment = trim($fragment);
                 next unless $fragment;
-                # Accept an order fragment matching a column name, with
-                # asc|desc optionally following (to specify the direction)
-                if (grep($fragment =~ /^\Q$_\E(\s+(asc|desc))?$/, @columnnames, keys(%$columns))) {
-                    next if $fragment =~ /\brelevance\b/ && !$fulltext;
-                    push(@order, $fragment);
+                my ($column_name, $direction) = split_order_term($fragment);
+                $column_name = translate_old_column($column_name);
+
+                # Special handlings for certain columns
+                next if $column_name eq 'relevance' && !$fulltext;
+                                
+                # If we are sorting by votes, sort in descending order if
+                # no explicit sort order was given.
+                if ($column_name eq 'votes' && !$direction) {
+                    $direction = "DESC";
+                }
+
+                if (exists $columns->{$column_name}) {
+                    $direction = " $direction" if $direction;
+                    push(@order, "$column_name$direction");
                 }
                 else {
                     my $vars = { fragment => $fragment };
@@ -953,58 +877,13 @@ if ($order) {
 
 if (!$order) {
     # DEFAULT
-    $order = "bugs.bug_status, bugs.priority, map_assigned_to.login_name, bugs.bug_id";
+    $order = "bug_status,priority,assigned_to,bug_id";
 }
 
-# Make sure ORDER BY columns are included in the field list.
-foreach my $fragment (split(/,/, $order)) {
-    $fragment = trim($fragment);
-    if (!grep($fragment =~ /^\Q$_\E(\s+(asc|desc))?$/, @selectnames)) {
-        # Add order columns to selectnames
-        # The fragment has already been validated
-        $fragment =~ s/\s+(asc|desc)$//;
-
-        # While newer fragments contain IDs for aliased columns, older
-        # LASTORDER cookies (or bookmarks) may contain full names.
-        # Convert them to an ID here.
-        if ($fragment =~ / AS (\w+)/) {
-            $fragment = $1;
-        }
-
-        $fragment =~ tr/a-zA-Z\.0-9\-_//cd;
-
-        # If the order fragment is an ID, we need its corresponding name
-        # to be in the field list.
-        if (exists($columns->{$fragment})) {
-            $fragment = $columns->{$fragment}->{'name'};
-        }
-
-        push @selectnames, $fragment;
-    }
-}
-
-$db_order = $order;  # Copy $order into $db_order for use with SQL query
-
-# If we are sorting by votes, sort in descending order if no explicit
-# sort order was given
-$db_order =~ s/bugs.votes\s*(,|$)/bugs.votes desc$1/i;
-                             
-# the 'actual_time' field is defined as an aggregate function, but 
-# for order we just need the column name 'actual_time'
-my $aggregate_search = quotemeta($columns->{'actual_time'}->{'name'});
-$db_order =~ s/$aggregate_search/actual_time/g;
-
-# the 'percentage_complete' field is defined as an aggregate too
-$aggregate_search = quotemeta($columns->{'percentage_complete'}->{'name'});
-$db_order =~ s/$aggregate_search/percentage_complete/g;
-
-# Now put $db_order into a format that Bugzilla::Search can use.
-# (We create $db_order as a string first because that's the way
-# we did it before Bugzilla::Search took an "order" argument.)
-my @orderstrings = split(/,\s*/, $db_order);
+my @orderstrings = split(/,\s*/, $order);
 
 # Generate the basic SQL query that will be used to generate the bug list.
-my $search = new Bugzilla::Search('fields' => \@selectnames, 
+my $search = new Bugzilla::Search('fields' => \@selectcolumns, 
                                   'params' => $params,
                                   'order' => \@orderstrings);
 my $query = $search->getSQL();
diff --git a/collectstats.pl b/collectstats.pl
index 761c648c8..bcb0fac5b 100755
--- a/collectstats.pl
+++ b/collectstats.pl
@@ -561,7 +561,7 @@ sub CollectSeriesData {
         # login name or a renamed product or component, etc.
         eval {
             my $search = new Bugzilla::Search('params' => $cgi,
-                                              'fields' => ["bugs.bug_id"],
+                                              'fields' => ["bug_id"],
                                               'user'   => $user);
             my $sql = $search->getSQL();
             $data = $shadow_dbh->selectall_arrayref($sql);
diff --git a/duplicates.cgi b/duplicates.cgi
index 06334e22b..af239d632 100755
--- a/duplicates.cgi
+++ b/duplicates.cgi
@@ -199,14 +199,14 @@ if (scalar(%count)) {
         $params->param('product', join(',', @query_products));
     }
 
-    my $query = new Bugzilla::Search('fields' => [qw(bugs.bug_id
-                                                     map_components.name
-                                                     bugs.bug_severity
-                                                     bugs.op_sys
-                                                     bugs.target_milestone
-                                                     bugs.short_desc
-                                                     bugs.bug_status
-                                                     bugs.resolution
+    my $query = new Bugzilla::Search('fields' => [qw(bug_id
+                                                     component
+                                                     bug_severity
+                                                     op_sys
+                                                     target_milestone
+                                                     short_desc
+                                                     bug_status
+                                                     resolution
                                                     )
                                                  ],
                                      'params' => $params,
diff --git a/report.cgi b/report.cgi
index fd2f28943..2f950948a 100755
--- a/report.cgi
+++ b/report.cgi
@@ -102,53 +102,42 @@ else {
     }
 }
 
-my %columns;
-$columns{'bug_severity'}     = "bugs.bug_severity";        
-$columns{'priority'}         = "bugs.priority";
-$columns{'rep_platform'}     = "bugs.rep_platform";
-$columns{'assigned_to'}      = "map_assigned_to.login_name";
-$columns{'reporter'}         = "map_reporter.login_name";
-$columns{'qa_contact'}       = "map_qa_contact.login_name";
-$columns{'bug_status'}       = "bugs.bug_status";
-$columns{'resolution'}       = "bugs.resolution";
-$columns{'component'}        = "map_components.name";
-$columns{'product'}          = "map_products.name";
-$columns{'classification'}   = "map_classifications.name";
-$columns{'version'}          = "bugs.version";
-$columns{'op_sys'}           = "bugs.op_sys";
-$columns{'votes'}            = "bugs.votes";
-$columns{'keywords'}         = "bugs.keywords";
-$columns{'target_milestone'} = "bugs.target_milestone";
-# Single-select fields are also accepted as valid column names.
-my @single_select_fields =
-  grep { $_->type == FIELD_TYPE_SINGLE_SELECT } Bugzilla->active_custom_fields;
-
-foreach my $custom_field (@single_select_fields) {
-    my $field_name = $custom_field->name;
-    $columns{$field_name} = "bugs.$field_name";
-}
-
-# One which means "nothing". Any number would do, really. It just gets SELECTed
-# so that we always select 3 items in the query.
-$columns{''}                 = "42217354";
+# Valid bug fields that can be reported on.
+my @columns = qw(
+    assigned_to
+    reporter
+    qa_contact
+    component
+    classification
+    version
+    votes
+    keywords
+    target_milestone
+);
+# Single-select fields (custom or not) are also accepted as valid.
+my @single_selects = Bugzilla->get_fields({ type => FIELD_TYPE_SINGLE_SELECT,
+                                            obsolete => 0 });
+push(@columns, map { $_->name } @single_selects);
+my %valid_columns = map { $_ => 1 } @columns;
 
 # Validate the values in the axis fields or throw an error.
 !$row_field 
-  || ($columns{$row_field} && trick_taint($row_field))
+  || ($valid_columns{$row_field} && trick_taint($row_field))
   || ThrowCodeError("report_axis_invalid", {fld => "x", val => $row_field});
 !$col_field 
-  || ($columns{$col_field} && trick_taint($col_field))
+  || ($valid_columns{$col_field} && trick_taint($col_field))
   || ThrowCodeError("report_axis_invalid", {fld => "y", val => $col_field});
 !$tbl_field 
-  || ($columns{$tbl_field} && trick_taint($tbl_field))
+  || ($valid_columns{$tbl_field} && trick_taint($tbl_field))
   || ThrowCodeError("report_axis_invalid", {fld => "z", val => $tbl_field});
 
-my @axis_fields = ($row_field, $col_field, $tbl_field);
-my @selectnames = map($columns{$_}, @axis_fields);
+my @axis_fields = ($row_field || EMPTY_COLUMN, 
+                   $col_field || EMPTY_COLUMN,
+                   $tbl_field || EMPTY_COLUMN);
 
 # Clone the params, so that Bugzilla::Search can modify them
 my $params = new Bugzilla::CGI($cgi);
-my $search = new Bugzilla::Search('fields' => \@selectnames, 
+my $search = new Bugzilla::Search('fields' => \@axis_fields, 
                                   'params' => $params);
 my $query = $search->getSQL();
 
@@ -179,9 +168,9 @@ foreach my $result (@$results) {
     $col = ' ' if ($col eq '');
     $tbl = ' ' if ($tbl eq '');
 
-    $row = "" if ($row eq $columns{''});
-    $col = "" if ($col eq $columns{''});
-    $tbl = "" if ($tbl eq $columns{''});
+    $row = "" if ($row eq EMPTY_COLUMN);
+    $col = "" if ($col eq EMPTY_COLUMN);
+    $tbl = "" if ($tbl eq EMPTY_COLUMN);
     
     # account for the fact that names may start with '_' or '.'.  Change this 
     # so the template doesn't hide hash elements with those keys
diff --git a/template/en/default/list/table.html.tmpl b/template/en/default/list/table.html.tmpl
index 9b27b0094..153823269 100644
--- a/template/en/default/list/table.html.tmpl
+++ b/template/en/default/list/table.html.tmpl
@@ -87,11 +87,11 @@
       [% END %]
       <th colspan="[% splitheader ? 2 : 1 %]" class="first-child">
         [% desc = '' %]
-        [% IF (om = order.match("^bugs\.bug_id( desc)?")) %]
-          [% desc = ' desc' IF NOT om.0 %]
+        [% IF (om = order.match("^bug_id( DESC)?")) %]
+          [% desc = ' DESC' IF NOT om.0 %]
         [% END %]
         <a href="buglist.cgi?
-                  [% urlquerypart FILTER html %]&amp;order=bugs.bug_id[% desc FILTER url_quote %]
+                  [% urlquerypart FILTER html %]&amp;order=bug_id[% desc FILTER url_quote %]
                   [%-#%]&amp;query_based_on=
                   [% defaultsavename OR searchname FILTER url_quote %]">ID</a>
       </th>
@@ -130,20 +130,13 @@
 
 [% BLOCK columnheader %]
   <th colspan="[% splitheader ? 2 : 1 %]">
-    [% IF column.name.match('\s+AS\s+') %]
-      [%# For aliased columns, use their ID for sorting. %]
-      [% column.sortalias = id %]
-    [% ELSE %]
-      [%# Other columns may sort on their name directly. %]
-      [% column.sortalias = column.name %]
-    [% END %]
     [% desc = '' %]
-    [% IF (om = order.match("$column.sortalias( desc)?")) %]
-      [% desc = ' desc' IF NOT om.0 %]
+    [% IF (om = order.match("$id( DESC)?")) %]
+      [% desc = ' DESC' IF NOT om.0 %]
     [% END %]
-    [% order = order.remove("$column.sortalias( desc)?,?") %]
+    [% order = order.remove("$id( DESC)?,?") %]
     <a href="buglist.cgi?[% urlquerypart FILTER html %]&amp;order=
-      [% column.sortalias FILTER url_quote %][% desc FILTER url_quote %]
+      [% id FILTER url_quote %][% desc FILTER url_quote %]
       [% ",$order" FILTER url_quote IF order %]
       [%-#%]&amp;query_based_on=
       [% defaultsavename OR searchname FILTER url_quote %]">
@@ -208,6 +201,13 @@
         [%- get_status(bug.$column).truncate(abbrev.$column.maxlength, abbrev.$column.ellipsis) FILTER html %]
       [% ELSIF column == 'resolution' %]
         [%- get_resolution(bug.$column).truncate(abbrev.$column.maxlength, abbrev.$column.ellipsis) FILTER html %]
+
+      [%# Display the login name of the user if their real name is empty. %]
+      [% ELSIF column.match('_realname$') && bug.$column == '' %]
+        [% SET login_column = column.remove('_realname$') %]
+        [% bug.${login_column}.truncate(abbrev.$column.maxlength, 
+                                        abbrev.$column.ellipsis) FILTER html %]
+
       [% ELSE %]
         [%- bug.$column.truncate(abbrev.$column.maxlength, abbrev.$column.ellipsis) FILTER html -%]
       [% END %]
diff --git a/whine.pl b/whine.pl
index 3eb757dd4..d8c8e8f4c 100755
--- a/whine.pl
+++ b/whine.pl
@@ -425,16 +425,16 @@ sub run_queries {
         next unless $savedquery;    # silently ignore missing queries
 
         # Execute the saved query
-        my @searchfields = (
-            'bugs.bug_id',
-            'bugs.bug_severity',
-            'bugs.priority',
-            'bugs.rep_platform',
-            'bugs.assigned_to',
-            'bugs.bug_status',
-            'bugs.resolution',
-            'bugs.short_desc',
-            'map_assigned_to.login_name',
+        my @searchfields = qw(
+            bug_id
+            bug_severity
+            priority
+            rep_platform
+            assigned_to
+            bug_status
+            resolution
+            short_desc
+            assigned_to
         );
         # A new Bugzilla::CGI object needs to be created to allow
         # Bugzilla::Search to execute a saved query.  It's exceedingly weird,