Closed Bug 760075 Opened 12 years ago Closed 12 years ago

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

Categories

(Bugzilla :: Query/Bug List, defect)

4.2.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: mail, Assigned: mail)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 patch (obsolete) — Splinter Review
This patch is against trunk. It will also apply cleanly (albeit with offsets) against Bugzilla 4.2 branch.
Assignee: query-and-buglist → sgreen+mozilla
Status: NEW → UNCONFIRMED
Ever confirmed: false
Attachment #628688 - Flags: review?
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: blocking4.2.2+
Keywords: regression
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → Bugzilla 4.2
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;
        ...
(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.
(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 on attachment 628688 [details] [diff] [review]
v1 patch

r=glob
Attachment #628688 - Flags: review? → review+
Flags: approval?
Flags: approval4.2?
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.
Attachment #628688 - Flags: review-
Flags: approval?
Flags: approval4.2?
Flags: blocking4.4+
Attached patch v2 patchSplinter Review
Updated patch
Attachment #628688 - Attachment is obsolete: true
Attachment #630784 - Flags: review?
Comment on attachment 630784 [details] [diff] [review]
v2 patch

r=glob
Attachment #630784 - Flags: review? → review+
Flags: approval?
Flags: approval4.2?
Thanks for this fix. :)
Flags: approval?
Flags: approval4.2?
Flags: approval4.2+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.