67036 - Can't query for product, component, version names that have commas

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect, P2)

Description

[Jesse Bouwman]

Component names which contain commas seem to prevent queries by component from
obtaining any results.

Comment 1

[Jesse Ruderman]

So I guess component names shouldn't be allowed to have commas?

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Mass moving to new product Bugzilla...

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Matthew Tuck [:CodeMachine]]

Sounded like a security issue at first but isn't.

This seems to be because buglist.cgi uses the advanced query code behind the
scenes which probably separates on space and comma.

I suggest prohibiting commas is the appropriate solution ... users of advanced
query will want to use these as separators explicitly.

Comment 5

[Antonis Christofides]

This is similar to bug 179309.

Comment 6

[Dave Miller [:justdave]]

This also affect products and versions....

Comment 7

[Dave Miller [:justdave]]

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

Comment 8

[Dave Pifke]

Attachment: Patch to use Text::ParseWords instead of split()

Attached is a proposed patch against CVS HEAD.

It escapes commas to \, and then modifies the anyexact sub to use quotewords()
from Text::ParseWords (part of the standard Perl distribution) instead of
split().

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 142271 [details] [diff] [review]
Patch to use Text::ParseWords instead of split()

Setting review? so I remember to come back and look at it when I have time to
play.

Question about this:  is Text::ParseWords part of a core Perl 5.6.0
distribution?  If not, we need a version check for it added to checksetup.pl as
well.

Comment 10

[Dave Pifke]

I'm not sure how far back we support, but I just downloaded a copy of 5.005_03
and Text::ParseWords is in there.  I found some usenet posts indicating it was
present at least as far back as 5.004.

Comment 11

[Joel Peshkin]

These bugs appear to be abandoned.  Retargeting to 2.20

Comment 12

[Dave Pifke]

Not abandoned; waiting for patch review.

I don't have permission to retarget this bug to 2.18, but I'd really like to see
a fix for this issue incorporated into the next stable release.

Comment 13

[Dave Miller [:justdave]]

(In reply to comment #10)
> I'm not sure how far back we support, but I just downloaded a copy of 5.005_03
> and Text::ParseWords is in there.

OK, that's far enough for me. :)

Comment 14

[Bradley Baetz (:bbaetz)]

Don't you need to DBI escape this, instead?

Comment 15

[Joel Peshkin]

It does not seem to be the database getting confused.  Rather, the code is
concatenating a bunch of search criteria joined with commas and then later using
a split(',',$whatever) to seperate them.

Comment 16

[Marc Schumann [:Wurblzap]]

Dave: is there a reason not to split on /(?<!\\),/ instead of having to use 
Text::ParseWords?

Comment 17

[Dave Miller [:justdave]]

If I'm not mistaken, quotewords() also ignores delimiters that are inside
quotes....  which means if you wanted to search for something that did contain
commas, you could put quotes around it, and the quotes would get removed by
quotewords() and the comma within it would be part of that term isntead of
getting split.  This is kind of a cool concept.  That would automatically allow
for you to search things with commas in them that didn't match something in a
@legal_xxxx array, too. :)

Comment 18

[Marc Schumann [:Wurblzap]]

(In reply to comment #17)
> commas, you could put quotes around it, and the quotes would get removed by
> quotewords() and the comma within it would be part of that term isntead of

This is indeed pretty cool. Is there a parser in there?

I don't mind, but wouldn't it perhaps be easier then to bracket the parameters 
with quotes instead of escaping the commas?

Comment 19

[Frédéric Buclin]

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

Comment 20

[Max Kanat-Alexander]

Comment on attachment 142271 [details] [diff] [review]
Patch to use Text::ParseWords instead of split()

Looks like nobody ever got around to looking at this.

Marc, did you want to take a crack at it?

Comment 21

[Vlad Dascalu]

Comment on attachment 142271 [details] [diff] [review]
Patch to use Text::ParseWords instead of split()

bitrotten.

Comment 22

[Vlad Dascalu]

Attachment: Patch to use Text::ParseWords instead of split()

Comment 23

[Vlad Dascalu]

Comment on attachment 185708 [details] [diff] [review]
Patch to use Text::ParseWords instead of split()

The patch deals only with the anyexact case.

It ignores for example ",anywordssubstr" or the ",allwordssubstr" cases, which
are right below it.

Their "split" is hidden at the end of the Search.pm file, in subs like:

# Support for "any/all/nowordssubstr" comparison type ("words as substrings")
sub GetByWordListSubstr {


or


sub GetByWordList {


They have in them an implicit split which is based on commas as well (and
spaces).

They would need to be updated as well if we're taking this approach.

Comment 24

[Frédéric Buclin]

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

Comment 25

[Steven W. Orr]

I was going to submit a new bug but I found this one already filed. Is there any
way that this bug can get allocated for repair? I only ask since this was
reported   a mere four and a half years ago. The work we had to do to identify
the bug in the first place came to a total of about 3 man-hours of
principal-level engineers. I understand that that's how the ball bounces but I
just hate to find that it's the result of something that's been known for so long.

Comment 26

[Frédéric Buclin]

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

Comment 27

[Joel Peshkin]

I think the right approach for this is as follows....

The @funcdefs list should be redefined replacing the 
  "string1,string2,string3" => sub {} form with.....

  {
    key =>("string1", "string2", "string3"), 
    ref => sub {}
  }

%funcsbykey is used in 2 ways....  in all but one place it is used to directly look up the member of @funcdefs where string1 is null and we are looking only for we are looking only for a member of @funcdefs where string2 is exactly what we are looking for.  
In the remaining place, we are sequentially checking @funcdefs for the first entry where each string matches the corresponding portion of the chart row as follows...
                foreach my $key (@funcnames) {
                    if ("$f,$t,$rhs" =~ m/$key/) {
                        my $ref = $funcsbykey{$key};
If we replace this with a loop like...
                foreach my ($key, $ref) (@funcnames) {
                    if (($f eq '' || $f =~ m/$key[0]) 
                       && ($t eq '' || $t =~ m/$key[1]) 
                       && ($rhs eq '' || $rhs =~ m/$key[2])) {
                 etc...

Someone will have to do all of this and make sure the data structures are all correct, but it should work well.

Comment 28

[Joel Peshkin]

OK... more like this...

@funcdefs = (
    {
        key => ['^(?:assigned_to|reporter|qa_contact)$',
                '^(?:notequals|equals|anyexact)$',
                '^%group\\.(\\w+)%$'],
        ref => sub { the code....; }
    },
    {
        key => ['^(?:assigned_to|reporter|qa_contact)$',
                '^(?:notequals|equals|anyexact)$',
                '.*'],
        ref => sub { the code....; }
    },
etc...

Which begs the question...
shoudl the anchors go inside every regexp or should they go outside and require the use of wildcards where anchored strings are not wanted.  I am leaning towards the latter

Comment 29

[Joel Peshkin]

Ugh!!  The last thing I want to do is to change all these around programatically.  This filter does about 90% of it, but the spurious "}," at the beginning of funcdefs needs to be moved to the end.

#!/usr/bin/perl

while ($l = <>) {
    $l =~ s| "\^(.*),(.*)(?:,(.*))?" => sub |},
        {
            key => ['$1',
                    '$2',
                    '$3'],
            ref => sub |;
    $l =~ s| ",(.*)(?:,(.*))?" => sub |},
        {
            key => ['-----',
                    '$1',
                    '$2'],
            ref => sub |;
    print $l;
}

Comment 30

[Joel Peshkin]

Attachment: Initial version for testing -- still needs whitespace fix

This still needs a massive whitespace fix, but it is worth testing.

Comment 31

[Joel Peshkin]

Attachment: The full patch

Comment 32

[Joel Peshkin]

Attachment: Same as above, but without showing whitespace changes

Since there is a lot of whitespace only change here, this is a patch with that removed

Comment 33

[Joel Peshkin]

Comment on attachment 211533 [details] [diff] [review]
The full patch

There is still an additional issue....

We need to make the search forms use a list of quoted strings for multi-selects and permit the anyexact searches to use EITHER a list of comma-delimited strings or, if the list starts witha quotation mark, a comma-seperated list of quoted strings.

Comment 34

[Joel Peshkin]

Attachment: v3 - use quotewords also

Comment 35

[Frédéric Buclin]

Comment on attachment 211546 [details] [diff] [review]
v3 - use quotewords also

mkanat said Search.pm wasn't that hard to read. So I let him reviewing this one. ;)

Comment 36

[Max Kanat-Alexander]

Comment on attachment 211546 [details] [diff] [review]
v3 - use quotewords also

Oh, passing the buck. :-) justdave's the owner of Search.pm...

Comment 37

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 39

[Jean-Marc Desperrier]

On bmo, in the "Mozilla Localizations" product, the components "es-AR / Spanish, Argentina", "ja-JP, ja-JPM / Japanese", "en-GB / English, United Kingdom" and "en-ZA / English, South Africa" are hit by this bug. I have now idea how those working on those localizations can stand the situation.

I don't know why Joel gave up pushing this bug, but it seems all the patch requires is a review by justdave ? (if it didn't bit-rot already)

Comment 40

[Dave Miller [:justdave]]

Comment on attachment 211546 [details] [diff] [review]
v3 - use quotewords also

(In reply to comment #39)
> I don't know why Joel gave up pushing this bug, but it seems all the patch
> requires is a review by justdave ? (if it didn't bit-rot already)

Probably because the review request was made at a time when I was very buried with work stuff, and was lost in the pile by the time I ever had time to look at it.  Yes, this is definitely very bitrotted :(

Concept behind it looks fairly sound though, and now's the perfect time to play with such a major overhaul to the basic search infrastructure (3.1 dev period)

Comment 41

[Dave Miller [:justdave]]

as before, there were a lot of whitespace-only changes.  To see only the actual changes this patch was making:

cvs -q up -dPC -D"18 Dec 2005 19:24:32 -0000"
patch -p0 < v3patchfile
cvs -q diff -uw

Comment 43

[Max Kanat-Alexander]

You know, also, I'd be really happy if somebody actually *named* those subs and just put them together at the bottom of Search.pm in their own section. I think that would make Search.pm a lot easier to read. (That is, you'd have key => [/some regex/], sub => \&_some_sub_name.

Comment 45

[Jesse Clark]

Attachment: v4

Updated the previous patch for bitrot, and added tightly related fixes mentioned in in bugs 179309 and 340884. I tried naming all the anonymous funcs and moving them outside @funcdefs, but didn't find a way to do this that I felt actually improved readability.

I noticed that v3 did not require types to match at the start of the string, so "notequals" would match "equals". Matching at the start should be appropriate for any patterns that were written in the comma style.

Quotewords does not fail very gracefully when given a string ending in a backslash that is not escaped properly, and it is possible for this to be incorrectly reported as a "field_type_mismatch" error.

Comment 47

[Paul]

I just experienced this bug on 2.22.

It needs to have it's severity raised because it causes bugzilla to give incorrect results.

This *silently* causes Zarro Bugs Found to be returned makeing the searcher feel that the bugs in question are zero.  There is nothing that they can do about it either.  The admin can go change the version numbers with commas but the damage is already done.

Comment 48

[Jesse Clark]

Attachment: v5

This patch uses quotewords to parse strings with commas and adapts selectable params such as Status to the quotewords format.

It also creates a direct hash of search functions by type to avoid the confusion caused by concatenation of commas onto search types, and clears up a good deal of other comma-related cruft.

Compound search functions are not placed in the constant hash because they may need runtime information such as the list of multiselect fields.

Comment 49

[Jesse Clark]

Attachment: v6

there was a small typo in the previous version

Comment 50

[Jesse Clark]

Attachment: v7

Added more user error text per bug 340892.

Comment 51

[Frédéric Buclin]

Comment on attachment 299982 [details] [diff] [review]
v7

>         if ($key =~ /^[^,]*$/) {
>-            die "All defs in %funcs must have a comma in their name: $key";
>+            die "All function patterns must contain a comma: $key";
>+        }
>+        if ($key =~ /^[^\^]/) {
>+            die "All function patterns must begin with a carat: $key";
>         }
>         if (exists $funcsbykey{$key}) {
>-            die "Duplicate key in %funcs: $key";
>+            die "Duplicate function pattern: $key";
>         }

What's that? We should never call die() directly. We should always use Throw*Error() instead, especially because Throw*Error() will correctly rollback pending SQL changes.

Comment 52

[Jesse Clark]

Attachment: v8

replace "die" calls with "ThrowCodeError"

Comment 53

[Frédéric Buclin]

Comment on attachment 300100 [details] [diff] [review]
v8

>+            ThrowCodeError("illegal_search_pattern",
>+                { pat => $key, msg => "does not contain a comma." });
>+        }
>+        if ($key =~ /^[^\^]/) {
>+            ThrowCodeError("illegal_search_pattern",
>+                { pat => $key, msg => "does not begin with a carat." });
>         }
>         if (exists $funcsbykey{$key}) {
>+            ThrowCodeError("illegal_search_pattern",
>+                { pat => $key, msg => "is not unique." });
>         }

Do not hardcode strings in Search.pm. It's impossible to translate them. Define msg => 'no_comma', 'no_character', and 'not_unique' (or names like that) and in code-error:

bla bla ...
[% IF msg = 'no_comma' %]
  does not contain a comma.
[% ELSIF msg = 'no_character' %]
  does not begin with ....
[% ELSIF msg = 'not_unique' %]
  is not unique.
[% END %]

Comment 54

[Jesse Clark]

Attachment: v9

Moved all ThrowCodeError text to the template file.

Comment 55

[Max Kanat-Alexander]

Not a blocker--this isn't a regression. It could still make it into 3.2, though--it's a bug, not an enhancement.

Comment 56

[Jesse Clark]

Actually, this might be a good time to move ALL search function patterns into a constant, not just the directly indexed ones. However it would require special attention for runtime-only patterns like multiselects.

Comment 57

[Max Kanat-Alexander]

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

Okay, why not just remove the whole idea of using commas in the regexes? Couldn't you do two regex matches, one for the type and another for the field, if that particular field has an override for that type?

Also, it looks like there's a lot of cleanup in this patch. Could we move that cleanup to a different patch, since this one is already large enough without me having to figure out what's cleanup and what's not?

Comment 58

[Max Kanat-Alexander]

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

Jesse and I talked this over on IRC a while back, and came up with some alternate methods that would be better architecturally.

Comment 59

[Axel Hecht [:Pike]]

I'd be interested in an ETA for this. I just happened to hit this, and we've been using ',' in the component names for l10n quite extensively, sadly. See https://bugzilla.mozilla.org/describecomponents.cgi?product=Mozilla%20Localizations.

Comment 60

[Gervase Markham [:gerv]]

The Bugzilla component reorg (http://www.gerv.net/temp/bmo-reorg.html) eliminates commas from l10n for exactly this reason.

Gerv

Comment 61

[Reed Loden [:reed]]

This is definitely (at least) major severity considering we (bmo) had to explicitly work around this issue by renaming components that had commas in them. Not sure how you can say otherwise.

Comment 62

[Marc Schumann [:Wurblzap]]

I'm with Reed. This is something I need to tell every new product admin explicitly. And it's being forgotten just as quickly, because a short list of things is handy -- so I need to re-tell every time somebody puts up a new component with commas (or renames an existing one to have commas).

Comment 64

[Christian Westgaard]

Confirmed in Bugzilla 3.2.2

Comment 66

[Frédéric Buclin]

Jesse, any progress?

Comment 67

[Jesse Clark]

Attachment: v10

Here is a minimal patch to address the comma issue on the head. Select fields work automatically, and commas can be escaped in the boolean charts using a common format. I take extra care to make this format appear verbatim on the search results page.

Older branches will need to handle some fields a little differently, such as keywords and content.

Comment 68

[Frédéric Buclin]

(In reply to comment #67)
> Older branches will need to handle some fields a little differently, such as
> keywords and content.

No need to worry about older branches. We will only fix it on HEAD (and 3.6, if the patch applies cleanly).

Comment 69

[Frédéric Buclin]

Comment on attachment 438023 [details] [diff] [review]
v10

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

>+    my @words = &quotewords('[,\s]+', 0, $strs)
>+        or ThrowUserError("misquoted_words", {str => $strs});

I was unable to trigger this error. Is this doable by the user, or only if something went internally wrong? In the last case, it should be a code error rather than a user error.


>+    ThrowUserError("misquoted_words", {str => $strs}) unless scalar @list;

Why not returning an empty array instead of throwing an error?


Otherwise looks good and works fine. Boolean charts were already working fine, AFAICS, and now select fields are working correctly too. r=LpSolit

I will let mkanat have a look at the patch too, in case I missed something.

Comment 70

[Jesse Clark]

(In reply to comment #69)
> I was unable to trigger this error. Is this doable by the user, or only if
> something went internally wrong? In the last case, it should be a code error
> rather than a user error.

If you enter an unparseable string into a boolean chart, you will get this error. For example:
field: product
type: anyexact
value: "Product One", "Products Two, Three"\

will fail to parse because of the trailing backslash. Since this is user-entered data, an error message is appropriate.

> Why not returning an empty array instead of throwing an error?

Good idea, if something goes wrong with a select field, it was probably a code error or an HTTP injection, so a user error is not helpful.

Comment 71

[Jesse Clark]

Attachment: v11

Updates per Comment 70. Also removed some whitespace-only changes, to make review easier.

Comment 72

[Max Kanat-Alexander]

  Just a comment here so that everybody knows--Jesse and I talked about this on IM, and we decided that an architecturally-better solution for HEAD would be to use arrayrefs instead of comma-separated strings, thus generally eliminating this problem.

  For the branch (if we decide to take this for the branch), the quotewords solution is more likely to be taken, though.

Comment 73

[Max Kanat-Alexander]

Comment on attachment 438173 [details] [diff] [review]
v11

I'm going to cancel review on this until we have a patch for HEAD.

Comment 74

[Frédéric Buclin]

Jesse, could we get some traction on this bug, please? :)

Comment 75

[Max Kanat-Alexander]

(In reply to comment #74)
> Jesse, could we get some traction on this bug, please? :)

 FWIW, this bug will be solved auto-magically by some refactoring I am planning to do on 4.2.

Comment 76

[Frédéric Buclin]

So why not take his patch v11 for 4.0 only?

Comment 77

[Max Kanat-Alexander]

(In reply to comment #76)
> So why not take his patch v11 for 4.0 only?

  I'd consider that, sure. Once I have the 4.2 patch ready, I'll also review the 4.0 patch.

Comment 78

[Max Kanat-Alexander]

I'm going to work on this now for 4.2, and then once that is done, I'll review the 4.0 patch.

Comment 79

[Max Kanat-Alexander]

Attachment: Work In Progress (trunk)

Here's a work-in-progress patch for trunk. However, currently this patch has a bunch of extra stuff that's unrelated to this bug in it (other improvements to Search.pm). I'm going to strip out the unrelated stuff, fix any remaining bugs, and then post the finalized patch, later.

Comment 80

[Max Kanat-Alexander]

Attachment: Trunk Refactor, v1

All right, this fixes it for trunk by passing an array of values to the _anyexact operator instead of passing a string. (Strings are still allowed, however, for use in the boolean charts.) 

Standard parameters (not boolean chart parameters) from the URL are always passed as arrays, so we preserve the functionality even if there is just a single value passed.

Comment 81

[Max Kanat-Alexander]

This checkin fixes this bug on trunk, and now I'm going to try the 4.0 patch out.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search.pm
Committed revision 7380.

Comment 82

[Max Kanat-Alexander]

Comment on attachment 438173 [details] [diff] [review]
v11

Interestingly, this patch still applies just fine on 4.0.

The new xt/search.t test says that this breaks anyexact searches for dates, like creation_ts and delta_ts, and for the longdesc field. I think it's because you split on spaces and commas both.

I think it might be possible to simply hack in an array-based solution for the anyexact charts only, for 4.0, which I'm going to try.

Comment 83

[Max Kanat-Alexander]

Attachment: v12 (4.0)

Okay, this fixes the problem on 4.0, and passes xt/search.t. It's a specific hack for the anyexact fields that appear as multi-selects on query.cgi.

Comment 84

[Frédéric Buclin]

Comment on attachment 458003 [details] [diff] [review]
v12 (4.0)

This fixes the problem, and the buglist now contains expected bugs, but the data displayed before the buglist header, where arguments passed to buglist.cgi are displayed, are all empty. This only affects this patch. Bugzilla 4.1 is fine.

Comment 85

[Frédéric Buclin]

Do not forget to grant approval for trunk, as you already committed the patch. ;) We are so close to get it work on 4.0 that I will block 4.0 on it. Do you plan to backport it to 3.6.x too?

Comment 86

[Max Kanat-Alexander]

(In reply to comment #85)
> Do not forget to grant approval for trunk, as you already committed the patch.
> ;) 

  Ah, thanks, yeah. :-)

> We are so close to get it work on 4.0 that I will block 4.0 on it.

  Sounds reasonable.

> Do you plan to backport it to 3.6.x too?

  No--I checked if the 4.0 patch applies, and it doesn't, so I'd have to write a different patch that I'm not sure would work. In particular, 3.6 still uses commas in func_defs, and I'm not sure how that would affect things (and it would be a lot more complex I suspect). I think it's probably better to keep 3.6 as it is than to risk any serious search regression on a stable branch.

Comment 87

[Frédéric Buclin]

(In reply to comment #86)
> I think it's probably better to keep
> 3.6 as it is than to risk any serious search regression on a stable branch.

Yeah, I have the same feeling about it. :)

Comment 88

[Max Kanat-Alexander]

Attachment: v13 (4.0)

Ah, thanks for catching that. :-) This fixes the search description.

Comment 89

[Frédéric Buclin]

Comment on attachment 459198 [details] [diff] [review]
v13 (4.0)

Works fine, thanks. r=LpSolit

Comment 90

[Max Kanat-Alexander]

Great, thanks for the review. :-)

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

Comment 91

[Max Kanat-Alexander]

Added to the release notes in bug 604256.