297382 - Move the "Sort Order Determination" code out of buglist.cgi and into Search.pm

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Max Kanat-Alexander]

I moved a lot of the ORDER BY generation out of buglist.cgi and into Search.pm,
but there's still a lot of hacky stuff that's going on inside of buglist.cgi to
fix the ORDER BY into the right shape.

In general, we should just be able to pass the &order= parameter pretty much
straight into Search.pm and it should deal with everything.

Comment 1

[Frédéric Buclin]

AFAICS, this is fixed already. I see nowhere ORDER BY mentioned in buglist.cgi.

Comment 2

[Max Kanat-Alexander]

Look for $db_order and other things like that. That's what I'm talking about.

Comment 3

[Teemu Mannermaa (:wicked)]

This is part of the work I'm doing for bug 245375. Mkanat, are you still working on this? You still think Search.pm is the right place for all this ordering code?

Comment 4

[Max Kanat-Alexander]

Yeah, I'm not working on this anymore.

I think Search.pm is the right place for it, yeah, but it's possible that we might have to fix bug 344878 first, and then use some of that data from the database. I have no idea, though, really. :-) I just think that all that crazy ordering stuff should be more centralized and not in a script.

Comment 5

[Max Kanat-Alexander]

This basically works for me, now. The work done in 344878 and related bugs fixed it.

Comment 6

[Teemu Mannermaa (:wicked)]

Not really since nothing validates order parameter to Search.pm because that  code is still in buglist.cgi starting at line 812 (on trunk).

Comment 7

[Teemu Mannermaa (:wicked)]

Attachment: Move order translation and validation, V1

Comment 8

[Max Kanat-Alexander]

Hey, I just wanted to let you know that I really like this patch and I'm absolutely planning to review it, but right now I'm engaged in some heavy refactoring of Search.pm and I think it will conflict with this (and the refactoring *must* be complete before 4.2) so I may not get to this until after that refactoring settles down a bit.

Comment 9

[Teemu Mannermaa (:wicked)]

Okay, thanks for letting me know. Just so you know, this is blocking the patch in bug 366793 so no custom sort order in whining until this is done. :)

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Okay, thanks for letting me know. Just so you know, this is blocking the patch
> in bug 366793 so no custom sort order in whining until this is done. :)

  Okay. That's definitely a patch that people want, so I will keep this on my radar.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 517255 [details] [diff] [review]
Move order translation and validation, V1

Review of attachment 517255 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good! Sorry it took me so long to get to it. :-) A few things to fix before checkin:

::: Bugzilla/Search.pm
@@ +870,5 @@
> +        my @order = map { ($self->_validate_order_column($_)) }
> +                        $self->_input_order;
> +        $self->{valid_order} = \@order;
> +    }
> +    return @{ $self->{valid_order} };

I don't think this needs to be cached, it can be generated on every call to _valid_order, unless profiling shows that this is otherwise important to performance.

@@ +887,5 @@
> +    $field = translate_old_column($field);
> +
> +    # Only accept valid columns
> +    if (!exists COLUMNS->{$field}) {
> +        push(@{ $self->{'invalid_order_columns'} }, $order_item);

Don't use the hash element, use the accessor. Also, perhaps we should do this? $self->invalid_order_columns($order_item).

Comment 12

[Teemu Mannermaa (:wicked)]

Attachment: Move order translation and validation, V1.1

Could you verify that I understood your comment correctly? I now modeled public invalid_order_columns sub like search_description sub as that seemed like what you were after.

I didn't remove the cache because there's a secondary reason for having that. This way the invalid_order_columns is only generated once (avoiding duplicating invalid column lists) even if _valid_order sub is called multiple times (and it normally always is).

Other way to handle that would have been to reset the hash each time _valid_order sub is called but I don't think that's a clean solution. And I believe that you wouldn't have liked this direct hash access either. :)

Comment 13

[Max Kanat-Alexander]

(In reply to comment #12)
> Could you verify that I understood your comment correctly? I now modeled
> public invalid_order_columns sub like search_description sub as that seemed
> like what you were after.

  Uh, no, all I asked for was two small changes in the way the internal code worked.

> I didn't remove the cache because there's a secondary reason for having
> that. This way the invalid_order_columns is only generated once (avoiding
> duplicating invalid column lists) even if _valid_order sub is called
> multiple times (and it normally always is).

  Is a performance problem or some actual logic problem?

Comment 14

[Teemu Mannermaa (:wicked)]

(In reply to comment #13)
>   Uh, no, all I asked for was two small changes in the way the internal code
> worked.

Yeah, and that required to change the way the invalid_order_columns sub worked so that it can set the value and not directly access it's hash. Or what hash element did you refer to? Can't be COLUMNS one since I can't see any accessor for it and everywhere else it's used directly like that..

> > I didn't remove the cache because there's a secondary reason for having
> > that. This way the invalid_order_columns is only generated once (avoiding
>   Is a performance problem or some actual logic problem?

Yes, if _valid_order logic gets executed more than once, the detected invalid columns get duplicated in the hash. Either the caching is needed or reset of the hash in _valid_order to avoid this duplication effort.

Why it's such a big problem to cache this? Memory issues?

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> Yeah, and that required to change the way the invalid_order_columns sub
> worked so that it can set the value and not directly access it's hash.

  Ah, okay. Thanks for the explanation.

> Yes, if _valid_order logic gets executed more than once, the detected
> invalid columns get duplicated in the hash. Either the caching is needed or
> reset of the hash in _valid_order to avoid this duplication effort.

  Oh, hmm, okay.

> Why it's such a big problem to cache this? Memory issues?

  It's not a big problem, I've just recently decided that I'd rather not cache things in Search.pm if I don't have to. (I'm worried about it hiding or creating subtle bugs that users of Search objects might not expect.)

Comment 16

[Frédéric Buclin]

Cancelling approval+ as a new patch is uploaded.

Comment 17

[Frédéric Buclin]

Too late and too invasive for 4.2, I think.

Comment 18

[Teemu Mannermaa (:wicked)]

Attachment: Move order translation and validation, V1.2

I'm not sure what in my answers made you think v1.1 was bigger change that it really was and that led you believing you need more time (or something) to review it again. Hopefully this version looks easier so we can move along with the blocked Whining change. :)

After I thought and reread your review comments from v1.0, I think this is actually what you had in mind. It does leave out the "perhaps do this" part of the review as that one needs the changed invalid_order_columns() sub that can also change the hash contents when there's a param.

The valid_order cache part is still there since I think you agreed it's required to avoid resetting the hash each time it's regenerated.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 568545 [details] [diff] [review]
Move order translation and validation, V1.2

Review of attachment 568545 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Search.pm
@@ +747,5 @@
> +
> +sub order {
> +   my ($self) = @_;
> +   return $self->_valid_order;
> +}

Okay, so I have some thoughts:

(1) Don't cache _valid_order.
(2) Don't have validate_order_column populate invalid_order columns. That's an unexpected side effect of calling an accessor.
(3) Have invalid_order_columns populate itself by running validate_order_column against every column itself, every time it's called.
(4) Eliminate relevance during _validate_order_column, so that you don't have to keep checking for it everywhere else.

@@ +834,5 @@
>  
>      my @select_columns;
>      foreach my $column ($self->_input_columns, $self->_extra_columns) {
> +        # Relevance column can be used only with one or more fulltext searches.
> +        next if ($column eq 'relevance' && !COLUMNS->{$column}->{is_usable});

How about just checking if it has a ->{name} instead of adding is_usable?

@@ +942,5 @@
>  
>      my ($field, $direction) = split_order_term($order_by_item);
>      
> +    # Relevance column can be used only with one or more fulltext searches.
> +    return if ($field eq 'relevance' && !COLUMNS->{$field}->{is_usable});

What are you returning there? Should it perhaps be ()--explicitly the empty list?

::: buglist.cgi
@@ +698,4 @@
>      }
>  }
>  
> +my @order;

Having an array and a scalar with the same name gets pretty confusing. Perhaps call one @order_columns?

@@ +721,5 @@
>              last ORDER;
>          };
>          do {
> +            # A custom list of columns. Bugzilla::Search will validate items.
> +            @order = split(/,\s*/, $order);

I think you want \s* on both sides of the comma, no?

Comment 20

[Teemu Mannermaa (:wicked)]

Attachment: Move order translation and validation, V2

This should address all the review comments.

Comment 21

[Max Kanat-Alexander]

Comment on attachment 577164 [details] [diff] [review]
Move order translation and validation, V2

Review of attachment 577164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!! Thank you so much, wicked! :-)

Comment 22

[Teemu Mannermaa (:wicked)]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 8022.

Comment 23

[Max Kanat-Alexander]

Hooray!! :-D