Stephen Lee Reporter
	Description • 24 years ago

If I create a query on the boolean chart area of the query form including:

[Keywords]  [contains regexp]
OR
[Keywords]  [does not contain regexp]

and enter a regexp, (e.g. ".*" for a test)

The web form comes back with the error: "Unknown keyword named .*"

ISTM that it should search the keywords table for all keywords containing the 
given regexp, and find all bugs [not] containing those keywords. This would 
allow related keywords with a similar base to be defined and searched for on 
this or other bugzilla installations.

E.g. looking at the main mozilla.org bugzilla keywords list, "KB.*", "ja.*" 
might be useful...

	Stephen Lee Reporter
	Comment 2 • 24 years ago

Attached patch for buglist.cgi appears to work on our own internal bugzilla 
system. Based on most recent downloadable release (2.10) as I haven't set up 
CVS and I doubt it would work through our firewall/web proxy.

May want tidying up by someone who knows Perl and/or bugzilla coding style 
better.

	Stephen Lee Reporter
	Comment 3 • 24 years ago

I've also created a patch to query.cgi to advertise the regexp functionality on 
the main query form, using further menu options of
"Any keyword matching regexp set" and "No keyword matching regexp set"

	Dave Miller [:justdave]
	Comment 5 • 24 years ago

moving to real milestones...

	Andreas Franke (gone)
	Comment 6 • 23 years ago

-> Bugzilla product, Query component, reassigning.

	Gervase Markham [:gerv]
	Comment 7 • 23 years ago

Asa: do we want regexp matching on keywords? I think it'll just complicate and
confuse.

Gerv

	Asa Dotzler [:asa]
	Comment 8 • 23 years ago

gerv, certainly. the boolean chart is for power users. if you can't understand
(or aren't interested in) what a keyword regexp match gets you then you
shouldn't be using the boolean tool :) I'd like it. makes queries for ns.* and
mozilla.* that much easier.

	Myk Melez [:myk] [@mykmelez]
	Comment 10 • 23 years ago

Comment on attachment 56625 [details] [diff] [review]
updated unified patch

>+                     if($keyword =~ $v) {
>+                         push(@klist, $keyword);
>+                     }

Optional: Make this briefer and easier to read by using the postfix syntax:

push(@klist, $keyword) if $keyword =~ $v;


>+                     return Error("No keywords matched regexp <code>$v</code>.\n" .
>+                                  "<P>The legal keyword names are\n" .
>+                                  "<A HREF=describekeywords.cgi>" .
>+                                  "listed here</A>.\n");

Use DisplayError instead of return Error, escape the regexp with html_quote() before 
displaying it, and rephrase the error message so you can linkify "legal keyword names"
rather than "listed here", f.e.:

No keywords matched regexp <code>$v</code>.  View <a href="describekeywords">legal keyword names</a>.

	Stephen Lee Reporter
	Comment 11 • 23 years ago

Modifications suggested by myk appear to be based on proposed changes done but 
not yet checked in. Current CVS has error handling code similar to that quoted, 
which changes in the manner stated above in bug 103778.

Adding dep for bug 103778, to avoid creating inconsistent schema.

According to attachment 56666 [details] [diff] [review] (on that bug), the new phrasing for standard 
keywords is currently:

+                     my $htmlv = html_quote($v);
+                     DisplayError(qq|There is no keyword named 
<code>$htmlv</code>.
+                                     To search for keywords, consult the
+                                    <a href="describekeywords.cgi">list of 
legal keywords</a>.|);

So for this bug, I guess we would copy this, replacing "There is no keyword 
named" with "No keyword matched regexp".

	Myk Melez [:myk] [@mykmelez]
	Comment 12 • 23 years ago

>Modifications suggested by myk appear to be based on proposed changes done but 
>not yet checked in. Current CVS has error handling code similar to that quoted, 
>which changes in the manner stated above in bug 103778.

DisplayError is already in CVS as a global function defined in CGI.pl, so you
can use it in this code.  I'd rather this bug not depend on bug 103778, since
this is a small fix and can go in quickly, while bug 103778 is a large change
that may require layers of review and refinement.

Post an updated patch and I'll review, and once it gets second review/check-in
I'll update the patch on bug 103778 with this fix.


>According to attachment 56666 [details] [diff] [review] (on that bug), the new phrasing for standard 
>keywords is currently:
  .....
>So for this bug, I guess we would copy this, replacing "There is no keyword 
>named" with "No keyword matched regexp".

Sounds good to me.

	Stephen Lee Reporter
	Comment 14 • 23 years ago

As per comments above from Myk, I've created an updated patch using the new 
error reporting mechanism and error text, and removed the dep on bug 103778.

I just modified Myk's updated unified patch, as I don't currently have CVS set 
up, but patch should be just as good as a freshly-generated one.

Using DisplayError / exit appears to work as stated without additional code 
that I had assumed was also part of these changes. Applying this patch on its 
own will make it the only use of DisplayError, but bug 103778 will fix the rest 
of the file in line with this.

	Dave Miller [:justdave]
	Comment 15 • 23 years ago

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

	Jouni Heikniemi
	Comment 16 • 23 years ago

Comment on attachment 56749 [details] [diff] [review]
Patch updated in line with changes included in bug 103778

The query.cgi modifications need redoing because of the templatization.

	Stephen Lee Reporter
	Comment 17 • 22 years ago

Updated this patch to work with 2.16. Changed the wording of the options in
line with the other options on the menu in the default template.

The back-end (buglist.cgi) is much the same and could be done independently of
the UI changes, such that the functionality is only available via boolean
chart.

	Stephen Lee Reporter
	Comment 18 • 22 years ago

Comment on attachment 104489 [details] [diff] [review]
Patch V4: Based on 2.16 codebase

*sigh*

It seems that the code that this patches isn't even present on the tip in
buglist.cgi.

Looks like Bug 158474 is the culprit.

If anyone else wants to do a patch, feel free. I'll most likely wait for the
next stable release.

	Dave Miller [:justdave]
	Comment 19 • 21 years ago

Pushing out bugs that aren't blockers.  If someone's working on one of these, we
can move it back.

	Max Kanat-Alexander
	Comment 20 • 20 years ago

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to nobody@bugzilla.org or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

	Stephen Lee Reporter
	Comment 21 • 20 years ago

Updated patch will "automatically" be created when I upgrade to 2.18+

Due to current workload and priorities, this may not happen for a while, and 
someone else may wish to update this simple patch to a current version in the 
meantime. Reassigning to default.

	Frédéric Buclin
	Comment 22 • 19 years ago

*** Bug 318748 has been marked as a duplicate of this bug. ***

	Rémi Zara
	Comment 23 • 18 years ago

Updated the previous patch from Stephen Lee to cvs tip.

	Joel Peshkin
	Comment 24 • 18 years ago

Comment on attachment 238810 [details] [diff] [review]
updated patch

There should be no reason to add special logic for this.  The boolean charts already support ..

"Keywords" "Contains the Regexp" 
and
NOT( "Keywords" "Contains the Regexp" )
 
You should implement this by pushing the appropriate specification from the hard-coded charts onto the boolean chart list the same way everything else works.

Also, the rest of the REGEXP logic in Bugzilla s done by the SQL server rather than in perl.  There are inconsitencies between the two tat we do not want to expose to the user.

All you should have to do is update the template for the search form and update...

    if ($params->param('keywords')) {
        my $t = $params->param('keywords_type');
        if (!$t || $t eq "or") {
            $t = "anywords";
        }
        push(@specialchart, ["keywords", $t, $params->param('keywords')]);
    }


to use the existing verbs.  Note that, if you want to find bugs where there is no keyword matching the regexp, you will have to set negation on the specialchart rather than using notregexp.

	Rémi Zara
	Comment 25 • 18 years ago

(In reply to comment #24)
> (From update of attachment 238810 [details] [diff] [review] [edit])
> There should be no reason to add special logic for this.  The boolean charts
> already support ..
>
> "Keywords" "Contains the Regexp" 
> and
> NOT( "Keywords" "Contains the Regexp" )
> 

I still get the behavior of comment #0 in boolean charts without the patch. Are you saying that this should work ?

	Joel Peshkin
	Comment 26 • 18 years ago

(In reply to comment #25)

> 
> I still get the behavior of comment #0 in boolean charts without the patch. Are
> you saying that this should work ?
> 

The problem is here....

         "^keywords,(?!changed)" => sub {
             my @list;
             my $table = "keywords_$chartid";
             foreach my $value (split(/[\s,]+/, $v)) {
                 if ($value eq '') {
                     next;
                 }
                 my $keyword = new Bugzilla::Keyword({name => $value});
                 if ($keyword) {
                     push(@list, "$table.keywordid = " . $keyword->id);
                 }
                 else {
                     ThrowUserError("unknown_keyword",
                                    { keyword => $v });
                 }
             }
             my $haveawordterm;
             if (@list) {

That whole first section should only run if the operation is one of the exact matches it can handle... otherwise, it should just fall right through and use the normal verb handlers after joining in the keywords table.

	Joel Peshkin
	Comment 27 • 18 years ago

More specifically, that section needs to be wholly rewritten.  Look at how the commenter logic works for an example.  Start by creating a rule for 
     "^keywords,(?!changed)" => sub {
 that should push 2 entries on to @supptables (one to join the bugs to the keywords and the second to join the keywords to their names) and then let the regular logic handle all of the verbs.

Once you have that working, then you can put a handler earlier on the list for 
"^keywords,(?anyexact|substring)" => sub { 
 that can look up the keywords in advance and substitute its own logic.

If you want the regexp logic to be especially fast, you will need to create a routine similar to ListIDsForEmail except for handling keywords.  Then, the actual search becomes one where you just push a single table into supptables with keyword.id IN(list) in the ON clause.

handling the NOT Regexp case requires bug 353053 to be solved.  Don't try to shortcut it.

	Rémi Zara
	Comment 28 • 18 years ago

This patch tries to maintain the existing behavior when doing exact searches on keywords. This is because the current code sometimes uses the bugs.keywords cache, and not the keyword table.

It adds a behavior similar to what is described in the first paragraph of comment #27 for "fuzzy" matches (substrings, regexps).

	Max Kanat-Alexander
	Comment 29 • 18 years ago

Comment on attachment 243900 [details] [diff] [review]
Yet another try

I won't be able to get to this.

	Frédéric Buclin
	Comment 30 • 17 years ago

Comment on attachment 243900 [details] [diff] [review]
Yet another try

jjclark, could you have a look at this patch? Probably bitrotten by your big patch, but having a look at the logic would be great.

	Jesse Clark Assignee
	Comment 31 • 16 years ago

Comment on attachment 243900 [details] [diff] [review]
Yet another try

This patch works as intended and can be applied to branches if complex keyword searches are desired. I don't really like adding regex to the main keyword search select without adding all search types, but I think the long-term solution to this should be to add the keyword chooser widget to the search page.

	Jesse Clark Assignee
	Comment 32 • 16 years ago

Updated patch for current tip with no functionality changes. This is the version I have tested.

	Jesse Clark Assignee
	Comment 33 • 16 years ago

oops, v8 contained unrelated changes in MySQL.pm

	Stephen Lee Reporter
	Comment 34 • 16 years ago

As original reporter ... finally upgrading from 2.16.x, but afraid I'll be reneging on the "promise" made in comment 21 that an updated patch would be "automatically" created, as I'll be converting most existing "keywords" into "flags" (which already allow search by regexp, and offer additional useful functionality).

For my own usage, this will make the original requirement obsolete...

	Frédéric Buclin
	Comment 35 • 16 years ago

Comment on attachment 243900 [details] [diff] [review]
Yet another try

bitrotten, especially the code in search/form.html.tmpl which has been moved into global/field-descs.none.tmpl, see bug 463002.

	Frédéric Buclin
	Comment 36 • 16 years ago

Comment on attachment 334586 [details] [diff] [review]
v9

bitrot :-/

	Kent Rogers [:mrbball]
	Comment 37 • 16 years ago

If I'm not mistaken, the standard regexp searching capability in 
Search.pm (in Bugzilla 3.2.X) will correctly handle keywords, as
long as _keywords_nonchanged() is NOT called.

The one-line change to Search.pm included here should take care of
that.  The previous suggested change to form.html.tmpl is also nice
(so that Boolean Charts aren't necessary) and is also included here.

This patch is applicable to Bugzilla 3.2.3.  Apparently some things
have moved around more recently (as mentioned in comment 35) which
make at least the form.html.tmpl piece N/A.  I do not have a newer
version of the code base to be able to suggest the necessary changes
for the trunk.

	Max Kanat-Alexander
	Comment 38 • 15 years ago

Kent: If you want your patch to get reviewed and checked in to Bugzilla, the thing to do is to follow our development process, as described here:

  http://wiki.mozilla.org/Bugzilla:Developers

Sorry nobody mentioned this earlier! :-) I am just seeing this bug for the first time, myself.

	Max Kanat-Alexander
	Comment 39 • 15 years ago

I unbitrotted Remi/Jesse's patch, and it worked perfectly against the current HEAD. The unbitrotting changes are really tiny, so I'm just going ahead and granting review.

	Max Kanat-Alexander
	Comment 40 • 15 years ago

The patch seems to apply fine all the way back to 3.4, and technically this is a bug fix, though it's somewhat of a large patch to fix a bug, so I want LpSolit's opinion on whether or not we should backport it.

	Jesse Clark Assignee
	Comment 41 • 15 years ago

Thanks for the update, Max!

	Frédéric Buclin
	Comment 42 • 15 years ago

(In reply to comment #40)
> The patch seems to apply fine all the way back to 3.4, and technically this is
> a bug fix, though it's somewhat of a large patch to fix a bug, so I want
> LpSolit's opinion on whether or not we should backport it.

This patch does two things: fixing the "contains regexp" part in boolean charts, and adding two new items to the "keywords" text field at the top of the query page. The first part can be seen as a bugfix, but the last part is clearly an enhancement, which may confuse users (3.4.5 doesn't have them, 3.4.6 does?!). As the "doesn't match regexp" action doesn't really work as expected when a bug has at least two keywords (it catches the other keyword, and reports the bug anyway despite it contains the keyword you tried to exclude), I prefer to not include this patch for the 3.4 branch.

To the one doing the checkin: please fix Rémi's first name in the patch. It's not valid UTF8. Also, who should get considered as the author of the patch here? I see several people involved.

	Max Kanat-Alexander
	Comment 43 • 15 years ago

I fixed the name in the source, on checkin.

You can actually specify --author multiple times, so I just did that. :-) I ordered them differently on each branch, so a different author will show up in the short revision log for each branch, but both authors are actually listed if you view the revision details, on both branches.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Search.pm
modified template/en/default/search/form.html.tmpl
Committed revision 6967.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified Bugzilla/Search.pm
modified template/en/default/search/form.html.tmpl
Committed revision 6963.