Bug 578275: Search.pm: Fully generate the ORDER BY clause inside of an

accessor
r=mkanat, a=mkanat (module owner)

1 files changed, 57 insertions, 96 deletions

diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index a7c37b94e..fc5e8b3ec 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -544,9 +544,8 @@ sub _input_columns { @{ $_[0]->{'fields'} || [] } }
 sub _extra_columns {
     my ($self) = @_;
     # Everything that's going to be in the ORDER BY must also be
-    # in the SELECT. We make a copy of input_order here so that modifications
-    # to extra_columns don't modify input_order.
-    $self->{extra_columns} ||= [ $self->_input_order ];
+    # in the SELECT.
+    $self->{extra_columns} ||= [ $self->_input_order_columns ];
     return @{ $self->{extra_columns} };
 }
 
@@ -575,15 +574,6 @@ sub _select_columns {
     return @{ $self->{select_columns} };
 }
 
-# JOIN statements for the display columns. This should not be called
-# Until the moment it is needed, because _select_columns might be
-# modified by the charts.
-sub _select_column_joins {
-    my ($self) = @_;
-    $self->{display_column_joins} ||= $self->_build_select_column_joins();
-    return @{ $self->{display_column_joins} };
-}
-
 # This takes _select_columns and translates it into the actual SQL that
 # will go into the SELECT clause.
 sub _sql_select {
@@ -607,6 +597,11 @@ sub _sql_select {
 # The "order" that was requested by the consumer, exactly as it was
 # requested.
 sub _input_order { @{ $_[0]->{'order'} || [] } }
+# The input order with just the column names, and no ASC or DESC.
+sub _input_order_columns {
+    my ($self) = @_;
+    return map { (split_order_term($_))[0] } $self->_input_order;
+}
 
 # A hashref that describes all the special stuff that has to be done
 # for various fields if they go into the ORDER BY clause.
@@ -633,20 +628,44 @@ sub _special_order {
     return $self->{special_order};
 }
 
-##################################
-# Helpers for Internal Accessors #
-##################################
+sub _sql_order_by {
+    my ($self) = @_;
+    if (!$self->{sql_order_by}) {
+        my @order_by = map { $self->_translate_order_by_column($_) }
+                           $self->_input_order;
+        $self->{sql_order_by} = \@order_by;
+    }
+    return @{ $self->{sql_order_by} };
+}
+
+###################################
+# Internal Accessors: FROM clause #
+###################################
 
-sub _build_select_column_joins {
+# JOIN statements for the SELECT and ORDER BY columns. This should not be called
+# Until the moment it is needed, because _select_columns might be
+# modified by the charts.
+sub _select_order_joins {
     my ($self) = @_;
     my @joins;
     foreach my $field ($self->_select_columns) {
         my @column_join = $self->_column_join($field);
         push(@joins, @column_join);
     }
-    return \@joins;
+    foreach my $field ($self->_input_order_columns) {
+        my $join_info = $self->_special_order->{$field}->{join};
+        if ($join_info) {
+            my @join_sql = $self->_translate_join($field, $join_info);
+            push(@joins, @join_sql);
+        }
+    }
+    return @joins;
 }
 
+##################################
+# Helpers for Internal Accessors #
+##################################
+
 sub _column_join {
     my ($self, $field) = @_;
     my $join_info = COLUMN_JOINS->{$field};
@@ -682,6 +701,23 @@ sub _translate_join {
     return @join_sql;
 }
 
+sub _translate_order_by_column {
+    my ($self, $order_by_item) = @_;
+
+    my ($field, $direction) = split_order_term($order_by_item);
+    
+    $direction = '' if lc($direction) eq 'asc';
+    my $special_order = $self->_special_order->{$field}->{order};
+    # Standard fields have underscores in their SELECT alias instead
+    # of a period (because aliases can't have periods).
+    $field =~ s/\./_/g;
+    my @items = $special_order ? @$special_order : $field;
+    if (lc($direction) eq 'desc') {
+        @items = map { "$_ DESC" } @items;
+    }
+    return @items;
+}
+
 ###############
 # Constructor #
 ###############
@@ -706,8 +742,6 @@ sub init {
     $params->convert_old_params();
     $self->{'user'} ||= Bugzilla->user;
     my $user = $self->{'user'};
-    my @inputorder = @{ $self->{'order'} || [] };
-    my @orderby;
 
     my @supptables;
     my @wherepart;
@@ -1198,28 +1232,10 @@ sub init {
         }
     }
 
-    # The ORDER BY clause goes last, but can require modifications
-    # to other parts of the query, so we want to create it before we
-    # write the FROM clause.
-    foreach my $orderitem (@inputorder) {
-        $self->BuildOrderBy($orderitem, \@orderby);
-    }
-    # Now JOIN the correct tables in the FROM clause.
-    # This is done separately from the above because it's
-    # cleaner to do it this way.
-    foreach my $orderitem (@inputorder) {
-        # Grab the part without ASC or DESC.
-        my $column_name = split_order_term($orderitem);
-        if (my $join_info = $self->_special_order->{$column_name}->{join}) {
-            my @join_sql = $self->_translate_join($column_name, $join_info);
-            push(@supptables, @join_sql);
-        }
-    }
-
     my %suppseen = ("bugs" => 1);
     my $suppstring = "bugs";
     my @supplist = (" ");
-    foreach my $str ($self->_select_column_joins, @supptables) {
+    foreach my $str ($self->_select_order_joins, @supptables) {
 
         if ($str =~ /^(LEFT|INNER|RIGHT)\s+JOIN/i) {
             $str =~ /^(.*?)\s+ON\s+(.*)$/i;
@@ -1284,7 +1300,7 @@ sub init {
     }
     # 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) {
+    foreach my $item ($self->_sql_order_by) {
         my $column_name = split_order_term($item);
         if (my $order = $self->_special_order->{$column_name}->{order}) {
             push(@groupby, $order);
@@ -1297,8 +1313,8 @@ sub init {
         $query .= " HAVING " . join(" AND ", @having);
     }
 
-    if (@orderby) {
-        $query .= " ORDER BY " . join(',', @orderby);
+    if ($self->_sql_order_by) {
+        $query .= " ORDER BY " . join(',', $self->_sql_order_by);
     }
 
     $self->{'sql'} = $query;
@@ -1530,61 +1546,6 @@ sub IsValidQueryType
     return 0;
 }
 
-# BuildOrderBy - Private Subroutine
-# This function converts the input order to an "output" order,
-# suitable for concatenation to form an ORDER BY clause. Basically,
-# it just handles fields that have non-standard sort orders from
-# %specialorder.
-# Arguments:
-#  $orderitem - A string. The next value to append to the ORDER BY clause,
-#      in the format of an item in the 'order' parameter to
-#      Bugzilla::Search.
-#  $stringlist - A reference to the list of strings that will be join()'ed
-#      to make ORDER BY. This is what the subroutine modifies.
-#  $reverseorder - (Optional) A boolean. TRUE if we should reverse the order
-#      of the field that we are given (from ASC to DESC or vice-versa).
-#
-# Explanation of $reverseorder
-# ----------------------------
-# The role of $reverseorder is to handle things like sorting by
-# "target_milestone DESC".
-# Let's say that we had a field "A" that normally translates to a sort 
-# order of "B ASC, C DESC". If we sort by "A DESC", what we really then
-# mean is "B DESC, C ASC". So $reverseorder is only used if we call 
-# BuildOrderBy recursively, to let it know that we're "reversing" the 
-# order. That is, that we wanted "A DESC", not "A".
-sub BuildOrderBy {
-    my ($self, $orderitem, $stringlist, $reverseorder) = (@_);
-
-    my ($orderfield, $orderdirection) = split_order_term($orderitem);
-
-    if ($reverseorder) {
-        # If orderdirection is empty or ASC...
-        if (!$orderdirection || $orderdirection =~ m/asc/i) {
-            $orderdirection = "DESC";
-        } else {
-            # This has the minor side-effect of making any reversed invalid
-            # direction into ASC.
-            $orderdirection = "ASC";
-        }
-    }
-
-    # Handle fields that have non-standard sort orders, from $specialorder.
-    if ($self->_special_order->{$orderfield}) {
-        foreach my $subitem (@{$self->_special_order->{$orderfield}->{order}}) {
-            # DESC on a field with non-standard sort order means
-            # "reverse the normal order for each field that we map to."
-            $self->BuildOrderBy($subitem, $stringlist,
-                                $orderdirection =~ m/desc/i);
-        }
-        return;
-    }
-    # Aliases cannot contain dots in them. We convert them to underscores.
-    $orderfield =~ s/\./_/g if exists COLUMNS->{$orderfield};
-
-    push(@$stringlist, trim($orderfield . ' ' . $orderdirection));
-}
-
 # Splits out "asc|desc" from a sort order item.
 sub split_order_term {
     my $fragment = shift;