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

RESOLVED FIXED in Bugzilla 3.6

Status

()

defect
--
minor
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jruderman, Assigned: LpSolit)

Tracking

({regression})

Dependency tree / graph
Bug Flags:
approval +
approval4.0 +
approval3.6 +
blocking3.6 -
blocking3.4.7 -

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

610 bytes, patch
mkanat
: review+
Details | Diff | Splinter Review
Reporter

Description

9 years ago
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.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → 3.4.5
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
Assignee

Comment 2

9 years ago
In Bugzilla 3.6, you get an error:

Specified comparison type is not supported.

Quoted strings should not be parsed.
Flags: blocking3.6+
Flags: blocking3.4.7+
Target Milestone: --- → Bugzilla 3.4
  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.
Flags: blocking3.6+ → blocking3.6-

Updated

9 years ago
Depends on: 554819
Assignee

Comment 4

9 years ago
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.
Flags: blocking3.6- → blocking3.6+
(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.
I don't see this bug getting resolved on 3.4.x either, particularly since it's about to be locked to security fixes.
Flags: blocking3.6-
Flags: blocking3.6+
Flags: blocking3.4.7-
Flags: blocking3.4.7+
Target Milestone: Bugzilla 3.4 → ---

Updated

9 years ago
Severity: normal → minor
  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.
Assignee

Updated

9 years ago
Duplicate of this bug: 572514
Assignee

Comment 9

9 years ago
Posted patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #459404 - Flags: review?(mkanat)
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.

Updated

9 years ago
Depends on: 578494
Target Milestone: --- → Bugzilla 3.6
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?
Attachment #459404 - Flags: review?(mkanat) → review-
Assignee

Comment 12

9 years ago
(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.
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Assignee

Comment 13

9 years ago
(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.
Assignee

Comment 14

9 years ago
Posted patch patch, v2Splinter Review
Escape the minus sign in QS::splitString() only.
Attachment #459404 - Attachment is obsolete: true
Attachment #461969 - Flags: review?(mkanat)
Comment on attachment 461969 [details] [diff] [review]
patch, v2

Beautiful. :-)
Attachment #461969 - Flags: review?(mkanat) → review+

Updated

9 years ago
Flags: approval4.0+
Flags: approval3.6+
Flags: approval+
Assignee

Comment 16

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.