Closed Bug 490322 Opened 15 years ago Closed 13 years ago

"contains all of the words" (allwords) searches on keywords don't work in Bugzilla 3.6

Categories

(Bugzilla :: Query/Bug List, defect)

3.2.8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.6

People

(Reporter: filipe.brandenburger, Assigned: mkanat)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.9) Gecko/2009042208 CentOS/3.0.9-1.el5.centos Firefox/3.0.9
Build Identifier: version 3.2.3

Suppose you have a keyword "Testkeyword" and another keyword "Testkeyword.1" and have different bugs using those keywords.

If you start an advanced search and filter by Keywords: "contains all of the keywords" "Testkeyword", you will get the bugs with "Testkeyword.1" included in the results.

If you filter by Keywords: "contains any of the keywords" "Testkeyword" (just change *all* with *any*) it works as expected and shows only bugs with keyword "Testkeyword".

I looked at Bugzilla/Search.pm and I believe the problem is that _keywords_nonchanged is using the _allwords function which uses $$term, but $$term should probably not be used in that case (it should join @list with ANDs instead). I did not test that yet.


Reproducible: Always

Steps to Reproduce:
1. Create keywords Testkeyword and Testkeyword.1
2. Add keyword Testkeyword to bug X and keyword Testkeyword.1 to bug Y
3. Search by keyword Testkeyword, requesting all keywords
Actual Results:  
Bugs X and Y will be returned.

Expected Results:  
Only bug X should be returned.
Actually it's because _allmatches uses REGEXP on the keywords field instead of using the separate keywords table.

The REGEXP is '(^|[^a-z0-9])' . quotemeta($word) . '($|[^a-z0-9])' which means it will consider "." to be a delimiter (when in fact it is not).

I believe the proper solution would be using EXISTS subqueries.
Attached patch Patch to use EXISTS subqueries (obsolete) — Splinter Review
This patch changes Search.pm in _keywords_nonchanged to use EXISTS(...) subqueries instead of a LEFT JOIN for anywords and REGEXPs for allwords.

I tested it in my Bugzilla 3.2.3 running on Linux CentOS 5.3 with MySQL backend. I tested allwords, anywords and nowords and all seem to be working fine.
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 374786 [details] [diff] [review]
Patch to use EXISTS subqueries

Your patch does not specify file names and is therefore not usable as-is. Please upload a correctly formatted patch (see https://wiki.mozilla.org/Bugzilla:Patches), and remember to ask for review when attaching it, so that your patch shows up on peoples' radar (see http://www.bugzilla.org/docs/reviewer-list.html).
Attachment #374786 - Flags: review-
(In reply to comment #3)
> Your patch does not specify file names

Yes it does, it starts with the following two lines:
--- Bugzilla/Search.pm.orig     2009-01-04 13:21:05.000000000 -0500
+++ Bugzilla/Search.pm  2009-04-27 13:19:22.000000000 -0400

The only file changed by this patch is Bugzilla/Search.pm.

The patch was produced with "diff -u", it was not a recursive diff as only one file was changed and I did not want to have to create two copies of the tree to create a simple patch like that.

> and is therefore not usable as-is.

I don't have any problems applying it using "patch -p0".

> and remember to ask for review when attaching it,
> so that your patch shows up on peoples' radar

I am copying mkanat@bugzilla.org, justdave@bugzilla.org to the bug, I believe that is what you mean by asking for review.

---

P.S.: I'm just trying to help by sending the fix to a problem that was discovered locally so that others can benefit from it. I'm sorry, but I don't have time to go through hoops and recreate a patch (that anyone familiar with patches should be able to inspect and apply) in specific format just because it looks better or whatever reason. I do not expect to be supposed to know that I have to copy certain people or set certain flags for my bug to be seen, I would expect one of the Bugzilla developers to handle new bugs and dispatch them appropriately so that things get done, but judging by the fact that this was left alone for 3 months I believe you choose not to do that. If you make it hard to have this committed, I will have no problem to continue patching it here after each upgrade. I don't mean this to be a rant, please take this as a constructive criticism and think about changing the way you handle bug reports and patches so that you can actually make it easier rather than harder for people to contribute to your great project.
  Hey Filipe. We totally appreciate the patch, and we're sorry for the misunderstanding on the patch format. It seems that there's some bug in our patch reader code (or some malformation in your patch that I'm not seeing) that caused the "Diff" view of your attachment (which is how we do review) to say that it had no file name.

  I'll review your patch myself, right now. Our full development process for contributors is documented here, though, for future patches and if you're interested in how our review process works:

  http://wiki.mozilla.org/Bugzilla:Developers
I can confirm this bug on HEAD.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 3.4
Comment on attachment 374786 [details] [diff] [review]
Patch to use EXISTS subqueries

  I'm a little uncertain about the correctness of this patch, because theoretically one can do other charts with keywords than the boolean charts, including "contains any of the strings".

>-    my $table = "keywords_$$chartid";

  This is actually necessary for using multiple boolean charts.

>+            push(@list, "EXISTS (SELECT * FROM keywords WHERE keywords.bug_id = bugs.bug_id AND keywords.keywordid = " . $keyword->id . ")");

  I think you need to SELECT * FROM $table, but I'm not entirely certain.
Attachment #374786 - Flags: review-
(In reply to comment #4)
> Yes it does, it starts with the following two lines:

Yup, sorry for relying on the diff viewer there.
(In reply to comment #5)
>   Hey Filipe. We totally appreciate the patch, and we're sorry for the
> misunderstanding on the patch format.

Ok, that explains it... I seldom use the patch viewer, so I did not guess it could be that.

> It seems that there's some bug in our
> patch reader code (or some malformation in your patch that I'm not seeing) that
> caused the "Diff" view of your attachment (which is how we do review) to say
> that it had no file name.

Funnily enough, one of my users had hit this exact issue before. I reported it to the maintainer of PatchReader Perl module, but it looks like he is not very responsive... For the record, here is the bug report and the patch:
http://rt.cpan.org/Public/Bug/Display.html?id=47625

>   I'll review your patch myself, right now.

Great! Thanks a lot.

Regarding the other comments that the patch might not be 100% correct, you may be right, I have to say that I am not really a programmer (more of a sysadmin) and I certainly do not know the architecture of Bugzilla the way you do, but I know some SQL and managed to find a fix that worked for me... I hope it is useful as a starting point, but please take it with a grain of salt.

> Our full development process for
> contributors is documented here, though, for future patches and if you're
> interested in how our review process works:
> 
>   http://wiki.mozilla.org/Bugzilla:Developers

Ok, that is great, I will try to read it next time I stumble upon a bug.

Thanks!
Filipe
Bugzilla 3.4 is now restricted to security bugs. We will retarget this bug (to 3.6 or later) when it's fixed.
Target Milestone: Bugzilla 3.4 → ---
The "allwords" search is actually now broken, more or less because of this bug, since we removed the keywords cache. So fixing this bug will also fix the keywords cache problem.
Assignee: query-and-buglist → mkanat
Flags: blocking3.6.2+
Summary: Search by keywords using "contains all of the keywords" returns substring matches → "contains all of the words" (allwords) searches on keywords don't work in Bugzilla 3.6
Depends on: bz-search-args
Target Milestone: --- → Bugzilla 3.6
Flags: blocking3.6.2+ → blocking3.6.3+
Are Bugzilla 4.0 and 4.1 also affected by this bug?
(In reply to comment #12)
> Are Bugzilla 4.0 and 4.1 also affected by this bug?

Ah yes, they are, because Search.pm uses [:alnum], which sees e.g. "." and "-" as word separators, as already reported in comment 1. Not sure why this syntax is used as keywords are stored separately in the keywords table. So something like "^$kw$" should be used, I guess.

Note that the bug summary is a bit confusing, because searching on keywords works fine *except* when they contain characters such as "." or "-" in them *and* you have two keywords which are a substring of each other, which is IMO not too frequent.

Also, this bug is IMO not a hard blocker. This problem is already present in Bugzilla 3.2 (but was working fine in Bugzilla 3.0, because it used keyword IDs for the search). So this is not a recent regression.
Keywords: regression
Version: unspecified → 3.2.8
Flags: blocking4.0+
The fix is to convert it into a boolean chart, right?

&field0-0-0=keywords&type0-0-0=equals&value0-0-0=foo
&field1-0-0=keywords&type1-0-0=equals&value1-0-0=bar
No, the fix is to make "keywords" "allwords" work properly.
Max, any chance to see it fixed for 3.6.3 and 4.0rc1?
Flags: blocking3.6.3+ → blocking3.6.4+
3.6.4 is being released today, and we pushed this bug from release to release since mid-2010, probably because it's not a real blocker. I will leave the blocking4.0 flag alone, to still keep the bug in our radar. But we probably won't fix it for 3.6 as it will soon enter its security-fix only mode.
Flags: blocking3.6.4+
Attached patch v1, 4.0Splinter Review
Here's a patch that backports a bunch of stuff from trunk and then fixes this bug for 4.0. It may also apply on 3.6.
Attachment #374786 - Attachment is obsolete: true
Attachment #511877 - Flags: review?(LpSolit)
Comment on attachment 511877 [details] [diff] [review]
v1, 4.0

Glooooooob. You *know* you want to review this last 4.0 blocker.... :-)
Attachment #511877 - Flags: review?(bjones)
Comment on attachment 511877 [details] [diff] [review]
v1, 4.0

r=glob works as advertised
Attachment #511877 - Flags: review?(bjones) → review+
Awesome, thank you Byron. :-) I'm going to check this patch in for 3.6 and 4.0 right now, so that we unblock 4.0, and then I'll do the trunk patch myself later. (I'm not worried about the trunk patch right now because we're not doing a trunk release and I own Search.pm right now on trunk, so I can just r+ my own patch for this, in a few days or so.)

The same patch that's already on here applied to 3.6, but the xt/ part just didn't apply, because the xt/ test doesn't exist on 3.6.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified Bugzilla/Search.pm
modified template/en/default/global/user-error.html.tmpl
modified xt/lib/Bugzilla/Test/Search/Constants.pm
Committed revision 7555.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified Bugzilla/Search.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7239.
Flags: approval4.0+
Flags: approval3.6+
Attachment #511877 - Flags: review?(LpSolit)
(In reply to comment #22)
> Awesome, thank you Byron. :-) I'm going to check this patch in for 3.6 and 4.0
> right now, so that we unblock 4.0, and then I'll do the trunk patch myself
> later.

It would still be good to fix the bug on trunk asap, as we always do (we commit patches on all branches at once). As most of us test on trunk, if there is something wrong with this fix, we won't catch it on time.
Status: NEW → ASSIGNED
Now that Search.pm no longer calls unknown_keyword, we have to copy the link in object_does_not_exist. Stolen from upstream.
Attachment #512530 - Flags: review?(mkanat)
Attachment #512530 - Flags: review?(mkanat) → review+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/
modified template/en/default/global/user-error.html.tmpl
Committed revision 7240.
Could this be fixed in 4.1, please? We usually don't leave a bug fixed on some branches only and not on the trunk. No reason to make an exception here.
Attached patch trunk, WIP (obsolete) — Splinter Review
Here's a work-in-progress for the trunk. This seems to work in most cases, but there are very weird interactions with GROUP_CONCAT that I don't yet understand. I'm going to test against Pg to see if there's just some MySQL bug going on.
Attached patch trunk, v1Splinter Review
Okay, finally! :-) This patch not only fixes every single keywords search type on trunk, it also fixes every single multi_select search type and see_also search type. (The only ones that still are sometimes not correct are "changedfrom/changedto", but that's because of the structure of the bugs_activity table, not because of Search.pm.)
Attachment #514134 - Attachment is obsolete: true
Attachment #515886 - Flags: review+
Flags: approval+
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Search.pm
modified xt/lib/Bugzilla/Test/Search/Constants.pm
modified xt/lib/Bugzilla/Test/Search/FieldTestNormal.pm
Committed revision 7725.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 814361
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: