Last Comment Bug 578494 - QuickSearch for phrases is busted
: QuickSearch for phrases is busted
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.6.1
: All All
: -- major (vote)
: Bugzilla 3.6
Assigned To: Max Kanat-Alexander
: default-qa
Mentors:
https://bugzilla.mozilla.org/buglist....
: 578576 (view as bug list)
Depends on:
Blocks: bmo-regressions-1007 553884
  Show dependency treegraph
 
Reported: 2010-07-13 14:54 PDT by Jesse Ruderman
Modified: 2010-08-06 19:51 PDT (History)
12 users (show)
mkanat: approval+
mkanat: approval4.0+
mkanat: blocking4.0+
mkanat: approval3.6+
mkanat: blocking3.6.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (3.6 branch) (809 bytes, patch)
2010-07-13 21:16 PDT, Reed Loden [:reed] (use needinfo?)
mkanat: review-
Details | Diff | Review
v1 (1.96 KB, patch)
2010-07-19 21:04 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review
Pg: No shellwords, v1 (1.65 KB, patch)
2010-08-01 16:58 PDT, Max Kanat-Alexander
LpSolit: review+
Details | Diff | Review

Description Jesse Ruderman 2010-07-13 14:54:05 PDT
If I try to QuickSearch for a phrase, I get 200 random bugs that don't even contain the second word.  This almost prevents me from getting work done.
Comment 1 Max Kanat-Alexander 2010-07-13 20:11:02 PDT
reed: I'm not entirely sure that this is an upstream bug. Can you reproduce it on landfill? I'm seeing a lot of bugs like "my search returns random results" reported against bmo.
Comment 2 Reed Loden [:reed] (use needinfo?) 2010-07-13 21:16:00 PDT
Created attachment 457245 [details] [diff] [review]
patch - v1 (3.6 branch)

Here's a patch that should work for the 3.6 branch. 4.0 and trunk need a better fix.
Comment 3 Max Kanat-Alexander 2010-07-13 21:17:14 PDT
Comment on attachment 457245 [details] [diff] [review]
patch - v1 (3.6 branch)

This won't work on Pg, as mentioned on IRC:

 reed: The upstream problem is that we don't actually have a fulltext engine for Pg.
 reed: So we have to understand quotes inside of sql_fulltext or whatever it's called, in Bugzilla/DB.pm.
 reed: That could probably be accomplished by using splitwords from Text::ParseWords instead of just splitting the string.
Comment 4 Max Kanat-Alexander 2010-07-13 21:18:20 PDT
  Or shellwords, more specifically--that tends to work the most simply.
Comment 5 Reed Loden [:reed] (use needinfo?) 2010-07-13 21:26:29 PDT
Comment on attachment 457245 [details] [diff] [review]
patch - v1 (3.6 branch)

Landed on bmo 3.6 branch as a temp hack until a better fix is made.

Committing to: bzr+ssh://bzr.mozilla.org/bmo/3.6/
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7160.
Comment 6 Frédéric Buclin 2010-07-14 04:45:12 PDT
Looks like a dupe of bug 556579.
Comment 7 Max Kanat-Alexander 2010-07-14 09:05:30 PDT
(In reply to comment #6)
> Looks like a dupe of bug 556579.

  No, that bug only affects 4.0, and this bug is against 3.6.
Comment 8 Max Kanat-Alexander 2010-07-19 20:53:44 PDT
Before I go about this: I'm assuming that Oracle fulltext understands quotes mean "this is a phrase", yes?
Comment 9 Max Kanat-Alexander 2010-07-19 21:04:03 PDT
Created attachment 458553 [details] [diff] [review]
v1

This should fix it on 3.6, 4.0, and trunk, I'm pretty sure.
Comment 10 Max Kanat-Alexander 2010-07-19 21:41:50 PDT
*** Bug 578576 has been marked as a duplicate of this bug. ***
Comment 11 Reed Loden [:reed] (use needinfo?) 2010-07-22 14:04:26 PDT
Comment on attachment 458553 [details] [diff] [review]
v1

No time to get to this until minimum of next week.
Comment 12 Frédéric Buclin 2010-07-22 16:34:25 PDT
Comment on attachment 458553 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/Search/Quicksearch.pm'

>+sub _matches_phrase {
>+    $phrase =~ s/"/\\"/g;

Seems to work, but I'm not sure about to trigger this line as shellwords() splits phrases on quotes. r=LpSolit
Comment 13 Frédéric Buclin 2010-07-22 16:34:55 PDT
I didn't check if it applies on 4.0 and 3.6.
Comment 14 Max Kanat-Alexander 2010-07-22 18:47:56 PDT
Thanks! It should apply on 4.0 and 3.6 also--there haven't been any significant changes to that code since 3.6.
Comment 15 Max Kanat-Alexander 2010-07-22 18:51:38 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/DB.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7397.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/DB.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7341.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/
modified Bugzilla/DB.pm
modified Bugzilla/Search/Quicksearch.pm
Committed revision 7144.
Comment 16 Max Kanat-Alexander 2010-07-22 19:12:31 PDT
Something about this fix is breaking the fulltext search, according to the xt/search tests on our tinderboxen.
Comment 17 Max Kanat-Alexander 2010-08-01 16:58:55 PDT
Created attachment 461961 [details] [diff] [review]
Pg: No shellwords, v1

Okay, this fixes the problem with Pg. The problem was that shellwords absolutely can't handle unbalanced quotes--it returns an empty result if you do something like: "don't try this" for content/matches (without the double-quotes). So instead, I'm doing a hack just to allow Quicksearch to work properly on Pg.
Comment 18 Max Kanat-Alexander 2010-08-01 16:59:54 PDT
BTW, there's also currently some taint error on Pg for quicksearch, on HEAD. I have to find out what it is.
Comment 19 Frédéric Buclin 2010-08-01 18:13:48 PDT
Comment on attachment 461961 [details] [diff] [review]
Pg: No shellwords, v1

Seems to work fine. Let's hope we don't regress anything else. :) r=LpSolit
Comment 20 Max Kanat-Alexander 2010-08-01 18:17:48 PDT
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/DB.pm
Committed revision 7415.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/DB.pm
Committed revision 7358.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified Bugzilla/DB.pm
Committed revision 7151.
Comment 21 Daniel Veditz [:dveditz] 2010-08-05 16:26:11 PDT
This is "fixed" (upstream), do we need another bug to get this patch deployed so it's actually fixed for BMO users?
Comment 22 Max Kanat-Alexander 2010-08-05 16:59:28 PDT
(In reply to comment #21)
> This is "fixed" (upstream), do we need another bug to get this patch deployed
> so it's actually fixed for BMO users?

  Well, no, I think Mozilla just needs to employ some person who actually has time to work on bugzilla.mozilla.org. (Currently all maintenance is being done by volunteers, really.)
Comment 23 Dave Miller [:justdave] (justdave@bugzilla.org) 2010-08-06 19:51:01 PDT
If it got committed to the 3.6 branch upstream, then we should have it on b.m.o already.  I just merged it again yesterday when 3.6.2 released.

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