760075 - Software error when doing anywords or allwords search that starts or ends with a space

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Simon Green]

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

[Simon Green]

Attachment: v1 patch

This patch is against trunk. It will also apply cleanly (albeit with offsets) against Bugzilla 4.2 branch.

Comment 2

[Simon Green]

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

[:glob ✱]

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

[Simon Green]

(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

[:glob ✱]

(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

[:glob ✱]

Comment on attachment 628688 [details] [diff] [review]
v1 patch

r=glob

Comment 7

[Frédéric Buclin]

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 9

[Simon Green]

Attachment: v2 patch

Updated patch

Comment 10

[:glob ✱]

Comment on attachment 630784 [details] [diff] [review]
v2 patch

r=glob

Comment 11

[Frédéric Buclin]

Thanks for this fix. :)

Comment 12

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/Search.pm
Committed revision 8095.

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