Last Comment Bug 760075 - Software error when doing anywords or allwords search that starts or ends with a space
: Software error when doing anywords or allwords search that starts or ends wit...
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 4.2.1
: All All
: -- normal with 1 vote (vote)
: Bugzilla 4.2
Assigned To: mail
: default-qa
:
Mentors:
: 761300 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 05:42 PDT by mail
Modified: 2012-06-08 00:03 PDT (History)
4 users (show)
LpSolit: approval+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 patch (571 bytes, patch)
2012-05-31 05:44 PDT, mail
glob: review+
LpSolit: review-
Details | Diff | Splinter Review
v2 patch (787 bytes, patch)
2012-06-06 17:43 PDT, mail
glob: review+
Details | Diff | Splinter Review

Description mail 2012-05-31 05:42:07 PDT
In the advance search page, you can get a software error if you do an anywords or allwords search that starts or ends with whitespace.

For example:
https://landfill.bugzilla.org/bugzilla-4.2-branch/buglist.cgi?query_format=advanced&longdesc=%20test&longdesc_type=allwordssubstr

This obviously isn't desired behaviour and should be fixed.
Comment 1 mail 2012-05-31 05:44:35 PDT
Created attachment 628688 [details] [diff] [review]
v1 patch

This patch is against trunk. It will also apply cleanly (albeit with offsets) against Bugzilla 4.2 branch.
Comment 2 mail 2012-05-31 05:59:01 PDT
It was a toss up between putting this change in the _substring_terms sub, or _handle_chart sub. Considering that _handle_chart trims the value if it was an array, I think this is the correct place to put it.
Comment 3 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-05-31 08:29:22 PDT
i'm not sure about this fix; i'm concerned about possible side-effects of trimming all values via __handle_chart().

to me it looks like the problem is in _multiselect_multiple:

    my @words = $operator eq 'anyexact' ? $self->_all_values($args)
                                        : split(/[\s,]+/, $value);
    my @terms;
    foreach my $word (@words) {
        $args->{value} = $word;
        $args->{quoted} = $dbh->quote($word);
        push(@terms, $self->_multiselect_term($args));
    }

this loop should skip empty $word's...

    foreach my $word (@words) {
        next if $word eq '';
        $args->{value} = $word;
        ...
Comment 4 mail 2012-05-31 14:04:40 PDT
(In reply to Byron Jones ‹:glob› from comment #3)
> i'm not sure about this fix; i'm concerned about possible side-effects of
> trimming all values via __handle_chart().

As I mentioned in comment #2, it was a bit of a toss up about where to put this. The reason I put it where it is is due to the fact we were using trim on value if it was passed in as an array.

I'd be interested to know of a use cases where this could go wrong.
Comment 5 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-05-31 20:40:55 PDT
(In reply to Simon Green from comment #4)
> As I mentioned in comment #2, it was a bit of a toss up about where to put
> this. The reason I put it where it is is due to the fact we were using trim
> on value if it was passed in as an array.

after sleeping on this, i think your approach is better, and i retract my comment :)

> I'd be interested to know of a use cases where this could go wrong.

well, i was surprised to see a sql error generated as a result of a value /not/ being trimmed; even after the rewrite, which simplified things a lot, it's still a complex module :)
Comment 6 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-05-31 20:50:32 PDT
Comment on attachment 628688 [details] [diff] [review]
v1 patch

r=glob
Comment 7 Frédéric Buclin 2012-06-01 08:53:13 PDT
Comment on attachment 628688 [details] [diff] [review]
v1 patch

You are introducing another bug. To prevent "bug" from matching "bugzilla", one can type "bug " (a trick which I also use when I'm looking for flag but not flagtype), but due to trim(), this trick will no longer work. The issue is not about untrimmed values. The problem is about empty strings. So we should really fix this problem only.
Comment 8 Frédéric Buclin 2012-06-04 13:39:38 PDT
*** Bug 761300 has been marked as a duplicate of this bug. ***
Comment 9 mail 2012-06-06 17:43:32 PDT
Created attachment 630784 [details] [diff] [review]
v2 patch

Updated patch
Comment 10 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-06-06 23:49:13 PDT
Comment on attachment 630784 [details] [diff] [review]
v2 patch

r=glob
Comment 11 Frédéric Buclin 2012-06-07 11:40:04 PDT
Thanks for this fix. :)
Comment 12 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-06-08 00:03:31 PDT
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Search.pm
Committed revision 8095.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search.pm
Committed revision 8259.

Note You need to log in before you can comment on or make changes to this bug.