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

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Query/Bug List
--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: Frédéric Buclin)

Tracking

({regression})

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

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

610 bytes, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 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

7 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

Comment 3

7 years ago
  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

7 years ago
Depends on: 554819
(Assignee)

Comment 4

7 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+
Keywords: regression

Comment 5

7 years ago
(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

7 years ago
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

7 years ago
Severity: normal → minor

Comment 7

7 years ago
  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

7 years ago
Duplicate of this bug: 572514
(Assignee)

Comment 9

7 years ago
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.
Attachment #459404 - Flags: review?(mkanat)

Comment 10

7 years ago
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

7 years ago
Depends on: 578494
Target Milestone: --- → Bugzilla 3.6

Comment 11

7 years ago
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

7 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

7 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

7 years ago
Created attachment 461969 [details] [diff] [review]
patch, v2

Escape the minus sign in QS::splitString() only.
Attachment #459404 - Attachment is obsolete: true
Attachment #461969 - Flags: review?(mkanat)

Comment 15

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

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

Updated

7 years ago
Flags: approval4.0+
Flags: approval3.6+
Flags: approval+
(Assignee)

Comment 16

7 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: 7 years ago
Resolution: --- → FIXED
Blocks: 584956
You need to log in before you can comment on or make changes to this bug.