Allow multiple custom search criteria to match one flag or one attachment

NEW
Unassigned

Status

()

Bugzilla
Query/Bug List
P1
enhancement
7 years ago
4 years ago

People

(Reporter: Max Kanat-Alexander, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
Dependency tree / graph
Bug Flags:
blocking4.4 -

Details

Attachments

(2 obsolete attachments)

(Reporter)

Description

7 years ago
As part of the Search refactoring of Bugzilla 4.2, all Custom Search clauses were made totally independent. That means that now:

  "Attachment description" "contains" "foo"
  AND
  "Attachment filename" "contains" "bar"

Means: "Any bug where one attachment's description contains 'foo' and one attachment's description contains 'bar'."

This was done in order to make Custom Search behave totally predictably and extremely simply. This actually resolves many, many support requests and bugs that have been filed over the years about "I added two criteria and weird things happened!"

However, it's still important to be able to also say, "I want to find one attachment that matches these two [three, four, n] criteria." or "I want to find one flag that matches all these criteria." In particular, that Flags search feature has been asked for for a long time.

This bug tracks adding in a comprehensive functionality for this that will be far better than the hacky, hidden, cryptic functionality that boolean charts *sometimes* implemented for this.

My current proposal is that you can specify that you're searching an "attachment" and then specify several criteria for just that attachment. You can search "a bug" and specify several critera for that bug. You can search "a comment" and specify several criteria for that comment. And so on.

This functionality won't be available in Bugzilla 4.2, even though this does represent a regression for the people who were familiar with the secret field-associating behaviors of boolean charts. This work could take several months, and I don't want to hold up 4.2 for that reason.

However, all people concerned about this should rest assured that we very much *do* want this functionality, and it will be *much better* and *far more functional* than it was in Bugzilla 4.0 when it is completed.
(Reporter)

Comment 1

7 years ago
(In reply to Max Kanat-Alexander from comment #0)
> Means: "Any bug where one attachment's description contains 'foo' and one
> attachment's description contains 'bar'."

  This should read:

  "and one attachment's **filename** contains 'bar'."
(Reporter)

Updated

7 years ago
Blocks: 38487
(From Max Kanat-Alexander on bug 759999 comment #2)
> I had a plan for future versions of search to allow you to do criteria that
> explicitly depend on each other. For example, for this case the UI would
> look something like:
> 
> [ Change ] [ by ] [ is equal to ] [ rc5tv@home.com ]
>            [ date ] [ is less than ] [ 2008-02-01 ]
>            [ date ] [ is greater than or equal to ] [ 2008-01-01 ]
> 
> (The other options in the "Change" menu would be "Bug," "Attachment," and
> "Comment".)
> 
> Given my current availability, somebody else will have to implement that UI.

(From Max Kanat-Alexander on bug 759999 comment #3)
> The first step toward implementing that UI would be to implement a backend
> that can handle criteria like that, and deciding on how the URL parameters
> should be structured (keeping in mind that short URL parameters are
> preferred and that it should be backwards-compatible with what is there
> now). I'd really appreciate somebody working on this, although it would
> certainly be a fair bit of work (at least one entire release cycle for one
> engineer).
Summary: Allow multiple custom search criteria to match one flag or one attachment → Allow multiple custom search criteria to match one field

Comment 3

6 years ago
https://bugzilla.mozilla.org/buglist.cgi?field0-0-0=longdesc;type0-0-0=changedby;value0-0-0=HeroreV%40yahoo.com;field0-1-0=longdesc;type0-1-0=substring;value0-1-0=Components.interfaces

Should return only bug 292789 and bug 429070.  On the Bugzilla 4.2 staging server, it also returns bug 331259.

I do this kind of query almost once a day; losing it (as a regression from bug 638623) would be quite a blow :(

Note that filtering out the noise myself isn't an option, because the new query takes too long and times out when the commenter in question is a heavy bmo user like me.

Updated

6 years ago
Duplicate of this bug: 759999
Assignee: query-and-buglist → glob
Depends on: 780820
I've created bug 780820 to deal with a subset of this work.
Assignee: glob → nobody

Updated

6 years ago
Priority: -- → P1
Duplicate of this bug: 785565
No longer depends on: 780820
Assignee: nobody → glob
Blocks: 726710, 764897
Created attachment 675241 [details] [diff] [review]
patch v1

adds the ability to search for changes made to the same field.
Attachment #675241 - Flags: review?(dkl)
(In reply to Byron Jones ‹:glob› from comment #7)
> Created attachment 675241 [details] [diff] [review]
> patch v1
> 
> adds the ability to search for changes made to the same field.

Following warning from prove xt/search.t:

Use of uninitialized value $joiner in string eq at Bugzilla/Search.pm line 1650.
 at Bugzilla/Search.pm line 1650
	Bugzilla::Search::_custom_search() called at Bugzilla/Search.pm line 1590
	Bugzilla::Search::_params_to_data_structure() called at Bugzilla/Search.pm line 1572
	Bugzilla::Search::_charts() called at Bugzilla/Search.pm line 1559
	Bugzilla::Search::_charts_to_conditions() called at Bugzilla/Search.pm line 692
	Bugzilla::Search::sql() called at xt/lib/Bugzilla/Test/Search/FieldTest.pm line 533
	Test::Exception::lives_ok() called at xt/lib/Bugzilla/Test/Search/FieldTest.pm line 533
	Bugzilla::Test::Search::FieldTest::do_tests() called at xt/lib/Bugzilla/Test/Search/FieldTest.pm line 517
	Bugzilla::Test::Search::FieldTest::run() called at xt/lib/Bugzilla/Test/Search.pm line 888
	Bugzilla::Test::Search::run() called at xt/search.t line 36
Created attachment 675856 [details] [diff] [review]
patch v2

addresses undef warnings thrown by xt/ tests
Attachment #675241 - Attachment is obsolete: true
Attachment #675241 - Flags: review?(dkl)
Attachment #675856 - Flags: review?(dkl)
Flags: blocking4.4?

Comment 10

5 years ago
As discussed with glob on IRC, I will be happy to take it for 4.4 if it's reviewed on time and works fine, but I won't block 4.4 on it.
Flags: blocking4.4? → blocking4.4-
One issue I found when comparing queries on a non-patched trunk with a patched one:

http://centos/677757/buglist.cgi?o5=notequals&f1=OP&f0=OP&o2=notequals&f4=OP&v5=0&query_format=advanced&j1=OR&f3=CP&f2=dependson&j4=OR&f5=blocked&f6=CP&v2=0&f7

Without:

SELECT bugs.bug_id AS bug_id, bugs.bug_severity AS bug_severity, bugs.priority AS priority,
       bugs.bug_status AS bug_status, bugs.resolution AS resolution, map_product.name AS product,
       map_component.name AS component, CASE WHEN INSTR(CAST(map_assigned_to.login_name AS BINARY),
       CAST('@' AS BINARY)) != 0 THEN SUBSTR(map_assigned_to.login_name, 1, INSTR(CAST(map_assigned_to.login_name AS BINARY),
       CAST('@' AS BINARY)) - 1) ELSE map_assigned_to.login_name END AS assigned_to, bugs.short_desc AS short_desc, bugs.delta_ts AS changeddate
       FROM bugs LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id
       INNER JOIN products AS map_product ON bugs.product_id = map_product.id
       INNER JOIN components AS map_component ON bugs.component_id = map_component.id
       INNER JOIN profiles AS map_assigned_to ON bugs.assigned_to = map_assigned_to.userid
       WHERE bugs.creation_ts IS NOT NULL AND security_map.group_id IS NULL

       AND bugs.bug_id NOT IN (SELECT blocked FROM dependencies WHERE dependson = 0)
       AND bugs.bug_id NOT IN (SELECT dependson FROM dependencies WHERE blocked = 0)

       GROUP BY bugs.bug_id ORDER BY bug_id LIMIT 500 

Results: > 10000 (hit the internal limit)

With:

SELECT bugs.bug_id AS bug_id, bugs.bug_severity AS bug_severity, bugs.priority AS priority,
       bugs.bug_status AS bug_status, bugs.resolution AS resolution, map_product.name AS product,
       map_component.name AS component, CASE WHEN INSTR(CAST(map_assigned_to.login_name AS BINARY),
       CAST('@' AS BINARY)) != 0 THEN SUBSTR(map_assigned_to.login_name, 1, INSTR(CAST(map_assigned_to.login_name AS BINARY),
       CAST('@' AS BINARY)) - 1) ELSE map_assigned_to.login_name END AS assigned_to, bugs.short_desc AS short_desc, bugs.delta_ts AS changeddate
       FROM bugs LEFT JOIN bug_group_map AS security_map ON bugs.bug_id = security_map.bug_id
       INNER JOIN products AS map_product ON bugs.product_id = map_product.id
       INNER JOIN components AS map_component ON bugs.component_id = map_component.id 
       INNER JOIN profiles AS map_assigned_to ON bugs.assigned_to = map_assigned_to.userid
       WHERE bugs.creation_ts IS NOT NULL AND security_map.group_id IS NULL

       AND 1=2
       AND 1=2

       GROUP BY bugs.bug_id ORDER BY bug_id LIMIT 500;

Results: 0

The results should be the same. Strangely "AND 1=2 AND 1=2" is being added instead of the clauses regarding blocked/dependson so the results will always be 0.

dkl
I see what is happening from comment 11:

 sub build_subselect {
-    my ($outer, $inner, $table, $cond) = @_;
-    return "$outer IN (SELECT $inner FROM $table WHERE $cond)";
+    my ($outer, $inner, $table, $cond, $negate) = @_;
+    # Execute subselects immediately to avoid dependent subqueries, which are
+    # large performance hits on MySql
+    my $q = "SELECT $inner FROM $table WHERE $cond";
+    my $dbh = Bugzilla->dbh;
+    my $list = $dbh->selectcol_arrayref($q);
+    return "1=2" unless @$list;
+    return $dbh->sql_in($outer, $list, $negate);
 }

If the condition is negated "NOT IN(...)" maybe we should return empty string if !@$list

return if (!@$list && $negate);


dkl
Comment on attachment 675856 [details] [diff] [review]
patch v2

Review of attachment 675856 [details] [diff] [review]:
-----------------------------------------------------------------

Except for the minor issue mentioned in comment 11, this looks good to me and works with all my other testing executed. the other nits such as the addition of extra rows on <back> and the inability to remove rows are not regressions and can be fixed in other bugs. r=dkl
Attachment #675856 - Flags: review?(dkl) → review+

Updated

5 years ago
Status: NEW → ASSIGNED
Flags: testcase?
Flags: approval4.4+
Flags: approval+
Keywords: relnote

Comment 14

5 years ago
Err.... wait!

Comment 0 is about joining several criteria against the same object, such as attachments:

"I want to find one attachment that matches these two [three, four, n] criteria."

So for instance, I want the following:

Attachment creator contains bugzilla

AND

Attachment MIME type is equal to text/plain


Without your patch, they behave as described in comment 0, i.e. not bound to the same attachment. With your patch, it behaves exactly the same way, so there is no improvement. If I select "against the same field", it forces me to choose between Attachment creator and Attachment MIME type, but I cannot select both, i.e. it's really bound to the same field but not to the same object, which is too restrictive.

I see that glob reworded the bug summary to replace bug/attachment by field, which is only a subselect of the problem described here. This is not what this bug is about.

Please reopen bug 780820 (whose bug summary must be adapted to be less restrictive) and attach your patch there. But this bug is really about what comment 0 describes.
Flags: approval4.4+
Flags: approval+

Comment 15

5 years ago
(In reply to David Lawrence [:dkl] from comment #12)
> If the condition is negated "NOT IN(...)" maybe we should return empty
> string if !@$list
> 
> return if (!@$list && $negate);

Don't do that. If you do this change, then Bugzilla crashes if it doesn't match any bug.

Comment 16

5 years ago
Comment on attachment 675856 [details] [diff] [review]
patch v2

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

> sub build_subselect {
>-    my ($outer, $inner, $table, $cond) = @_;
>-    return "$outer IN (SELECT $inner FROM $table WHERE $cond)";
>+    my ($outer, $inner, $table, $cond, $negate) = @_;
>+    # Execute subselects immediately to avoid dependent subqueries, which are
>+    # large performance hits on MySql
>+    my $q = "SELECT $inner FROM $table WHERE $cond";
>+    my $dbh = Bugzilla->dbh;
>+    my $list = $dbh->selectcol_arrayref($q);
>+    return "1=2" unless @$list;
>+    return $dbh->sql_in($outer, $list, $negate);
> }

I don't like this subroutine. It generates an incredibly long list of bug IDs in the SQL query. Just append &debug=1 to the URL and see the output. I hope MySQL or any other DB server is clever enough to optimize subselects without having to pass a list with several thousands of items. Do you have data about what you argue here?

Moreover, the generated list has the same bug IDs several tens of times. Looks like a DISTINCT is missing in the SQL query. This must be fixed. As dkl's fix in comment 12 is incorrect (Bugzilla crashes with it), it looks like the problem described in comment 11 needs a new patch rather than a fix on checkin.
Attachment #675856 - Flags: review-

Updated

5 years ago
Keywords: relnote
Summary: Allow multiple custom search criteria to match one field → Allow multiple custom search criteria to match one flag or one attachment
(In reply to Frédéric Buclin from comment #16)
> I don't like this subroutine. It generates an incredibly long list of bug
> IDs in the SQL query. Just append &debug=1 to the URL and see the output. I
> hope MySQL or any other DB server is clever enough to optimize subselects
> without having to pass a list with several thousands of items. Do you have
> data about what you argue here?

yes, mysql isn't clever enough to optimise subselects; instead it will execute the subselect **for every row** even if it is lacking dependencies.

same query in 4.0 takes 7 seconds, 4.2 takes 223 seconds.

i don't think "not liking the long list of bug ids in the sql query" is a valid reason for rejecting this change, as that's purely hidden cosmetics.

> Moreover, the generated list has the same bug IDs several tens of times.
> Looks like a DISTINCT is missing in the SQL query. This must be fixed. As
> dkl's fix in comment 12 is incorrect (Bugzilla crashes with it), it looks
> like the problem described in comment 11 needs a new patch rather than a fix
> on checkin.

can do.  will move back to bug 780820 and morph.
(In reply to Frédéric Buclin from comment #16)
> This must be fixed. As
> dkl's fix in comment 12 is incorrect (Bugzilla crashes with it), it looks
> like the problem described in comment 11 needs a new patch rather than a fix
> on checkin.

Yeah I see that what should have been returned now is "1=1" and was just a possible
idea that was completely untested. 

dkl
Assignee: glob → nobody
No longer blocks: 726710, 764897
Depends on: 780820
Attachment #675856 - Attachment is obsolete: true

Comment 19

5 years ago
(In reply to Byron Jones ‹:glob› from comment #17)
> same query in 4.0 takes 7 seconds, 4.2 takes 223 seconds.

Which example did you use? I'm interested to do some comparisons myself.


> i don't think "not liking the long list of bug ids in the sql query" is a
> valid reason for rejecting this change, as that's purely hidden cosmetics.

That's not the reason of my r-. I denied review because dkl found a bug in your patch, and his proposed fixed made Bugzilla to crash. This is why I said a new patch was needed.
Status: ASSIGNED → NEW
Flags: testcase?
Target Milestone: Bugzilla 4.4 → ---

Updated

5 years ago
Blocks: 785565

Updated

4 years ago
No longer blocks: 785565

Updated

4 years ago
Assignee: nobody → query-and-buglist

Updated

4 years ago
Duplicate of this bug: 1048018
You need to log in before you can comment on or make changes to this bug.