553884 - Quicksearch incorrectly treats - in quotes as negation (e.g. "-6")

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Jesse Ruderman]

xpcshell "-6"

Should search for bugs that involve xpcshell and the string "-6", and find bug 548406.  Instead, it treats the "-6" as search term negation.

Comment 1

[Reed Loden [:reed]]

Not just '-'... '+' does the same thing, for example. I think it's probably a problem in QuickSearch.pm's splitString()... http://mxr.mozilla.org/bugzilla/source/Bugzilla/Search/Quicksearch.pm#495

Comment 2

[Frédéric Buclin]

In Bugzilla 3.6, you get an error:

Specified comparison type is not supported.

Quoted strings should not be parsed.

Comment 3

[Max Kanat-Alexander]

  I agree that this is a problem, but this isn't serious enough to warrant blocking a major release, particularly since it's not a regression--this must have been the case for many years now (possibly at least since Bugzilla 2.22, if not before).

  I've investigated the problem, and it's unlikely to have a reasonable resolution any time soon. If there were, it would involve some fair bit of rearchitecture and testing in Quicksearch.pm, to fix what is essentially a minor issue, because the subroutines in Quicksearch actually have no idea that input was quoted--they just get it as a "word". 

  Also, note that even were we to pass "-6" directly to the database, in 3.6 MySQL's fulltext search would again interpret the -6 as negation.

  So there's no reasonable resolution that I see happening any time soon.

Comment 4

[Frédéric Buclin]

No, this is a regression! You get an error in 3.6, which is not the case in older releases. Even if -6 is interpreted incorrectly, it shouldn't throw an error, see comment 2. So it blocks 3.6.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> No, this is a regression! You get an error in 3.6, which is not the case in
> older releases. Even if -6 is interpreted incorrectly, it shouldn't throw an
> error, see comment 2. So it blocks 3.6.

  Okay. The regression is a separate bug. I'll file it separately. It happens whether there are quotes or not, so it has nothing to do with Jesse's bug.

Comment 6

[Max Kanat-Alexander]

I don't see this bug getting resolved on 3.4.x either, particularly since it's about to be locked to security fixes.

Comment 7

[Max Kanat-Alexander]

  Oh, there might be a branch solution for this--we fix the urlencoding stuff in Quicksearch.pm to urlencode the minus sign if the incoming stuff was quoted. Don't know if that will work, though.

Comment 9

[Frédéric Buclin]

Attachment: patch, v1

This fixes the problem mentioned in this bug. I hope this doesn't regress anything else elsewhere, but I don't see why it would.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 459404 [details] [diff] [review]
patch, v1

Hmm. Actually, I'm pretty sure that the quoting fix that's pending review in bug 578494 will fix this, without us having to modify url_quote. So let's get that in first and then see if this is still a problem.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 459404 [details] [diff] [review]
patch, v1

  Oh, actually, I see what your patch is doing. I don't want to make that change in all of Bugzilla, though--I'm not sure what side effects it could have, and dashes are excluded from all the filters I've seen. (Also, we just talked about removing this function entirely and making it call Template::Filters::uri_filter.)

  Could we perhaps just make a change in splitString that does the regex just for "-", with an appropriate comment explaining why?

Comment 12

[Frédéric Buclin]

(In reply to comment #10)
> Hmm. Actually, I'm pretty sure that the quoting fix that's pending review in
> bug 578494 will fix this, without us having to modify url_quote.

No, it won't. I thought that too first, before testing. Unfortunately, it doesn't. :)


(In reply to comment #11)
>   Could we perhaps just make a change in splitString that does the regex just
> for "-", with an appropriate comment explaining why?

Oh, I think that's a good idea, and will prevent regressions. I can do that.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> > Hmm. Actually, I'm pretty sure that the quoting fix that's pending review in
> > bug 578494 will fix this, without us having to modify url_quote.
> 
> No, it won't. I thought that too first, before testing. Unfortunately, it
> doesn't. :)

Oh, hum, I had another bug in mind. Maybe this will fix it. We will see.

Comment 14

[Frédéric Buclin]

Attachment: patch, v2

Escape the minus sign in QS::splitString() only.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 461969 [details] [diff] [review]
patch, v2

Beautiful. :-)

Comment 16

[Frédéric Buclin]

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

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

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