Last Comment Bug 553884 - Quicksearch incorrectly treats - in quotes as negation (e.g. "-6")
: Quicksearch incorrectly treats - in quotes as negation (e.g. "-6")
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.4.5
: All All
: -- minor (vote)
: Bugzilla 3.6
Assigned To: Frédéric Buclin
: default-qa
Mentors:
https://bugzilla.mozilla.org/buglist....
: 572514 (view as bug list)
Depends on: 554819 578494
Blocks: 584956
  Show dependency treegraph
 
Reported: 2010-03-21 00:36 PDT by Jesse Ruderman
Modified: 2010-08-06 02:07 PDT (History)
6 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: approval3.6+
mkanat: blocking3.6-
mkanat: blocking3.4.7-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (683 bytes, patch)
2010-07-22 06:03 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2 (610 bytes, patch)
2010-08-01 18:57 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-03-21 00:36:20 PDT
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] (use needinfo?) 2010-03-21 00:52:00 PDT
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 2010-03-21 06:36:38 PDT
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 2010-03-24 18:05:04 PDT
  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 2010-03-25 05:16:29 PDT
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 2010-03-25 10:06:23 PDT
(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 2010-03-25 10:07:21 PDT
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 2010-04-02 05:12:03 PDT
  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 8 Frédéric Buclin 2010-06-16 13:50:51 PDT
*** Bug 572514 has been marked as a duplicate of this bug. ***
Comment 9 Frédéric Buclin 2010-07-22 06:03:27 PDT
Created attachment 459404 [details] [diff] [review]
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 2010-07-22 14:00:23 PDT
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 2010-07-22 15:03:17 PDT
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 2010-07-22 15:30:42 PDT
(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 2010-07-22 15:53:53 PDT
(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 2010-08-01 18:57:29 PDT
Created attachment 461969 [details] [diff] [review]
patch, v2

Escape the minus sign in QS::splitString() only.
Comment 15 Max Kanat-Alexander 2010-08-01 18:58:16 PDT
Comment on attachment 461969 [details] [diff] [review]
patch, v2

Beautiful. :-)
Comment 16 Frédéric Buclin 2010-08-01 19:06:15 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7417.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7360.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7153.

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