searching for 'flag' 'is' 'review+' AND 'attachment is obsolete' 'not equal' '1' doesn't work, because theres no link between the flag part of the query and the attachment part of the query. Not sure how to handle this with bug flags - we could LEFT JOIN from flags to attachments, but then the join for the obsolete bit wouldn't match, and we'd have duplicate table defs (we have a nopther bug somewhere where a similar situation occurs, so we may end up having to change the duplicate code) Thoughts?

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

joel, could you look at this one?

I think that when bbaetz doesn't know how to search for something, the answer is far from obvious :-) The problem is that the flag search code is a special case rather than being built into the normal boolean charts logic. I think this was done because our usual paradigm runs out of gas... Normally.... "summary" "contains" "foo" AND "priority" "equals" "P1" is simple enough and we have "summary" "contains" "foo" AND "priority" "equals" "P1" AND (chart) "cc" "contains" "justdave" "cc" "contains" "bugzilla" recognizing that we want both statements to be true OF THE SAME cclist entry since we can have more than one cc/comment/attachment on a single bug. When we get to flags, we can have more than one attachment per bug AND more than one flag per attachment. Our "add a new chart" mechanism runs out of gas when we do that because we cannot distinguish between multiple statments about a certain flag request or multiple requests on a single attachment. The curent code hacks around this and doers not fit neatly into boolean charts. The really right way to do this probaby requires making the charts multi-level. So, we could do things like... "summary" "contains" "foo" AND "priority" "equals" "P1" AND (chart/cc) "cc" "contains" "justdave" "cc" "contains" "bugzilla" AND (chart/attachment) "isobsolete" "equals" "1" (chart/flag) "requester" "contains" "bbaetz" "requestee" "contains" "bbaetz" etc... Kind of ugly, though...

joel, any chance to fix this bug soon? Not only is this feature really useful, but I also realized during QA tests that you could still search for flags set on private attachments even if you are not allowed to see them, probably because this link flag-attachment is missing.

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".

Can ths problem be solved by creating two separate "flag" query targets, say "bug flag" and "attachment flag", so that bbaetz's original query can become 'attachment flag' 'is' 'review+' AND 'attachment is obsolete' 'not equal' '1' mozilla developers have a difficult time finding reviewed patches. A patch may await a review for a year, and during that time it is easy to find, but when it gets reviewed, it drops off the radar, and is difficult to find. I have been using a query like this one: Attachment data changed by (me) And Attachment is patch is equal to 1 And Attachment is obsolete is not equal to 1 And Attachment description does not contain the string checked And Flag contains the string review+ And it suffers from the same problem as bbaetz's query: the flag check is not tied to the same attachment as the rest of the tests, resulting in false positives. I think this problem is sufficiently important to mozilla that this bug ought to be blocking SOMETHING.

Creating a pseudo-field for "attachment flag name" would also entail creating fields for attachment flag requestee and setter. It would be nicer to detect whether a flag applies to attachments based on the flag data, like so: > LEFT JOIN flags AS flags_0 ON bugs.bug_id = flags_0.bug_id >+ AND (flags_0.attach_id = attachments_0.attach_id >+ OR flags_0.attach_id IS NULL) but using the existing INNER JOIN on attachments would eliminate results that have no attachments for a query that was not referring to an attachment flag. Replacing that INNER JOIN with a LEFT JOIN could work but it would impact performance.

Applies the same changes to flag setter and requestee search as v1 does to flag name/status search.

Comment on attachment 304393 [details] [diff] [review] v2 >+ if (Bugzilla->params->{"insidergroup"} >+ && !Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"})) Write !Bugzilla->user->is_insider instead of these two lines. Much shorter and safer.

Search of attachment data should be a left join anyway, according to bug 306012

Alternatively, we could throw a user error for incomplete flag names, like we do for keywords.

(In reply to comment #11) > Alternatively, we could throw a user error for incomplete flag names, like we > do for keywords. Hum, no. We should be able to get all bugs with flags e.g. blocking-foo+ or approval-foo+ (e.g. in Core/Firefox/Thunderbird products) where "foo" can be anything.

Bugzilla 3.2 is restricted to security bugs only. Moreover, this bug is either assigned to nobody or got no traction for several months now. Rather than retargetting it at each new release, I'm clearing the target milestone and the bug will be retargetted to some sensible release when someone starts fixing this bug for real (Bugzilla 3.8 more likely).

A solution that fixes this problem and the one in the duplicate bug 329930 is worth USD $200 to me. Any takers?

Updated and tested with these cases: buglist.cgi?query_format=advanced&order=Bug%20Number&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B buglist.cgi?type0-1-0=notequals&query_format=advanced&value0-1-0=1&field0-1-0=attachments.isobsolete&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review%2B buglist.cgi?type1-0-0=notequals&field0-0-0=flagtypes.name&query_format=advanced&value1-0-0=1&type0-0-0=equals&value0-0-0=review%2B&field1-0-0=attachments.isobsolete Appears to work correctly with other combinations of these tables. At some points I join the same table twice in one function to avoid duplicating a join condition when the supptables are merged.

Comment on attachment 437741 [details] [diff] [review] v3 JOINing the same table twice with the same name concerns me; I think that's likely to cause errors about vague column names in the SELECT.

Lots of search functions join the same table with the same name but different conditions, then at the end we go through and build a single ANDed condition for each table. The only difference here is that I am doing it twice in the same function, so the resulting SQL doesn't have "(attachments_0.bug_id = bugs.bug_id)" twice. We could keep the attachments join simpler if we want to allow the redundant final condition, or we could relegate parsing it more carefully to a different bug. I like addressing it here because it is the most isolated.

Comment on attachment 437741 [details] [diff] [review] v3 The $attachments stuff for the flagtypes looks like it might modify the behavior of OR chart rows that also do things with attachments in the same chart. Is that not the case?

@jjclark Can you reassure me that this doesn't break OR charts on attachments?

It breaks OR charts. Searching for (review+ OR attachment non-obsolete) fails to find a bug that has a non-obsolete attachment with no flags, when that bug also has an obsolete attachment with review-. I am looking into this.

Comment on attachment 437741 [details] [diff] [review] v3 Okay, r- based on breaking OR charts.

In order to correctly express the join condition ($flags.attach_id = $attachments.attach_id OR $flags.attach_id IS NULL), I had to put it on the flags join instead of the attachments join, and make sure that attachments is defined first in the SQL. Now I am seeing correct behavior for OR charts as well.

Comment on attachment 443282 [details] [diff] [review] v4 Okay. Why "OR $flags.attach_id IS NULL"? Isn't LEFT JOIN already doing that by its very nature?

The full condition is LEFT JOIN flags AS $flags ON ($flags.bug_id = bugs.bug_id AND ($flags.attach_id = $attachments.attach_id OR $flags.attach_id IS NULL)) Without the last clause, the flag row would be NULL for non-attachment flags. This way we can use the same join condition for bug flags and attachment flags.

Comment on attachment 443282 [details] [diff] [review] v4 Okay. I think that your understanding of this section of the code probably exceeds mine or anybody else's, so I'm going to r+ this patch based on code inspection and your testing.

Because this involves so much complexity, I'm going to approve it for trunk only.

Nelson, if you were serious about that bounty, then feel free to test this on the bugzilla-tip demo install ( http://landfill.bugzilla.org/bugzilla-tip/ ) and if it's fixed, then Jesse is your bounty earner. Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Search.pm Committed revision 7147.

I was and am serious about the bug bounty. When I can do what I described in Bug 329930 comment 0 on BMO, the money will be on it's way. Jesse, are you in the USA? If so, it will be easy for me to get you the $$ if not, we'll have to figure out how to do it. Please write me in email.

I decline the bounty.

Added to the release notes in bug 604256.