252889 - Change Search.pm activity log joins to LEFT JOINS

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

This will make it possible to search correctly for inactivty as well activity 
OR comment, etc...

Comment 1

[Joel Peshkin]

Attachment: Patch to Search.pm -- actually simplifies

Comment 2

[Joel Peshkin]

Incidentally, this also extends the relative date capability to changedbefore
and changedafter searches.

Comment 3

[Joel Peshkin]

Attachment: v2

Comment 4

[Joel Peshkin]

Comment on attachment 154201 [details] [diff] [review]
v2

there.. remembered to add the new error message this time

Comment 5

[Jouni Heikniemi]

Joel, if you want a review for these patches, please write a short explanation
on what's going on here and what's your aim this time. I'd be happy to chip in
some r=s, but not so interested in guesswork on the thoughts behind your patch ;-)

Comment 6

[Joel Peshkin]

There are a whole flood of these coming.   Search.pm currently has too many
special cases because it is clumsy about the general case.

When we (implied inner) join in another table, we incorrectly assume that there
will be matching records in any bug we want to display.  That is incorrect when
we are using an OR clause and it is incorrect when we use negation.

So, what is written today as 

select foo from bugs, othertable WHERE  other_table_join_condition AND
other_table_criteria AND other_unrelated_conditions

needs to to be

select foo from bugs LEFT JOIN othertable ON other_tabel_join_condition AND
other_table_citeria WHERE (other_table.something IS NOT NULL) AND
other_unrelated_conditions

In this example, the 2 are equivalent in function and mysql4 is even smart
enough to optimize the LFT JOIN out.  mysql3 will still work, but will not
optimize it as well.  There is a consensus that, for 2.20, we will still work
with mysql3, but large sites will be expected to be on mysql4.

However, change the AND to an OR or surround a chunk of the boolean equation
with a NOT and all sorts of things break the old way.  The new way becomes

select foo from bugs LEFT JOIN othertable ON other_tabel_join_condition AND
other_table_citeria WHERE (other_table.something IS NOT NULL) OR
other_unrelated_conditions

and the results are exactly what you would expect them to be.

Comment 7

[Casey Klein]

This looks solid overall. It doesn't really hurt to revalidate the field name in
these rules, but FYI the name is currently checked at the beginning of the big
chart loop, so your UserErrors will never be thrown right now. 

Good call on Sqlify()ing the date values.

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 154201 [details] [diff] [review]
v2

I concur with Casey on the field name validation...  it's building the list for
the field validation at line 1044 (with this patch applied).  I would suggest
loading this hash with the fieldids instead of just setting it to 1 if the
field exists.  Then you can get the fieldids from within the other subs just by
looking it up in the hash instead of having to go back to the database again. 
I don't think we have anything with a fieldid of 0, so that should still work.

Otherwise this looks pretty good.  I have this applied to an install on
landfill...  I'm going to try to continue to play with it tomorrow and do some
performance testing and so forth.

Comment 9

[Joel Peshkin]

Attachment: v3

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 155882 [details] [diff] [review]
v3

First query I tried killed it.

cleared all contents of advanced query form, and filled into the boolean chart
section:

"Assigned To" "changed before" "2004-05-01 00:00:00"

Without the patch, this query works.  With the patch, it produces this error:

DBD::mysql::st execute failed: You have an error in your SQL syntax near 'AND
act_0.bug_when > '2004-05-01 00:00:00') LEFT JOIN bug_group_map  ON bug_grou'
at line 1 [for Statement "SELECT bugs.bug_id, bugs.bug_severity, bugs.priority,
bugs.bug_status, bugs.resolution, bugs.bug_severity, bugs.priority,
bugs.rep_platform, map_assigned_to.login_name, bugs.bug_status,
bugs.resolution, bugs.short_desc FROM bugs, profiles AS map_assigned_to  LEFT
JOIN bugs_activity act_0 ON (act_0.bug_id = bugs.bug_id AND act_0.fieldid = 
AND act_0.bug_when > '2004-05-01 00:00:00') LEFT JOIN bug_group_map  ON
bug_group_map.bug_id = bugs.bug_id  WHERE bugs.assigned_to =
map_assigned_to.userid AND bugs.assigned_to = map_assigned_to.userid AND
(((act_0.bug_when IS NOT NULL))) AND ((bug_group_map.group_id IS NULL)) GROUP
BY bugs.bug_id ORDER BY bugs.bug_id "] at Bugzilla/DB.pm line 62
	Bugzilla::DB::SendSQL('SELECT bugs.bug_id, bugs.bug_severity,
bugs.priority, bugs.bu...') called at /var/www/html/bzdave/buglist.cgi line 762

Also of note, in that error message, is it's checking for a date greater than
the one I entered, when I asked for "changed before", which suggests the
operator logic is backwards.

Comment 11

[Dave Miller [:justdave]]

Actually, to be fair, the query in question does not work without the patch,
either, but the reason it fails is different.  Without the patch, we get a
zero-results query.  Examining the SQL shows it's looking for
'map_assigned_to.login_name' in the fielddefs table, which of course, doesn't
exist, because fielddefs has it listed as 'assigned_to'.

Comment 12

[Dave Miller [:justdave]]

the logic in your patch for the operator direction *looks* correct...  the only
thing I can guess here is if $1 doesn't contain what we think it should at that
point.  (which is likely, as the regexp is not being run right then, but used to
look up the key elsewhere)

Comment 13

[Joel Peshkin]

Attachment: v4

Wow.... a lot of Search.pm was broken before.

It turns out that the reason $1 was not set to $t was because the code that
handles the supptable for assigned_to (as well as a whole bunch of others) was
improperly doing the supptable operation even when the activity log was needed.
 Since the activity log is text, not a key, that is incorrect.

So, that is generally straightened out and there is now an error handler in
case these functions are called that way again.

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 165124 [details] [diff] [review]
v4

wowsers.  this actually seems to fix some horkage, too.  We should put this in
2.18 as well.

Comment 15

[Dave Miller [:justdave]]

nah, patch doesn't apply cleanly to 2.18.  As it *is* a little invasive, I guess
it's just another excuse for people to upgrade to 2.20 :)

Comment 16

[Vlad Dascalu]

Justdave managed to be tricked someone to think this is a crucial patch or
something, so he said I can commit it. :-)

Thanks Joel, really impressive.

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.69; previous revision: 1.68
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.77; previous revision: 1.76
done

Comment 17

[Dave Miller [:justdave]]

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