Closed Bug 785565 Opened 12 years ago Closed 11 years ago

Search by change history between two dates doesn't give expected result

Categories

(Bugzilla :: Query/Bug List, defect)

4.2.2
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: benoit, Assigned: LpSolit)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

When searching for bug by "Search By Change History" between two dates Bugzilla 4.2.1 and 4.2.2 (at least) list bugs that doesn't match the request.
Example when searching for bug where Status has been RESOLVED between 2012/01/01 and 2012/01/31 i got bugs that where resolved years before, or that don't even have a change recorded on this timeframe.

This doesn't happens on the 4.0.7+ of bugzilla.mozilla.org.

Extract of the sql request generated by 4.2.2:
..
LEFT JOIN bugs_activity AS act_8_1 ON bugs.bug_id = act_8_1.bug_id AND act_8_1.fieldid = 8 AND act_8_1.added = 'RESOLVED' 
LEFT JOIN bugs_activity AS act_8_2 ON bugs.bug_id = act_8_2.bug_id AND act_8_2.fieldid = 8 AND act_8_2.bug_when >= '2012-01-01 00:00:00' 
LEFT JOIN bugs_activity AS act_8_3 ON bugs.bug_id = act_8_3.bug_id AND act_8_3.fieldid = 8 AND act_8_3.bug_when <= '2012-01-31 23:59:59' 

WHERE bugs.creation_ts IS NOT NULL 
...
	AND bugs.delta_ts >= '2012-01-01' 
	AND act_8_1.bug_when IS NOT NULL 
	AND act_8_2.bug_when IS NOT NULL 
	AND act_8_3.bug_when IS NOT NULL 
..

We see three different JOINs with each a part of the filter, if i'm not misleading that request say "give me bugs whose Status ( has been changed to RESOLVED or has changed after the 2012/01/01 or has changed before the 2012/01/31 )" which is more or less all resolved bugs (modulo the filtering made by bugs.delta_ts).

Request generated on BMO:
	LEFT JOIN bugs_activity AS actcheck ON (actcheck.bug_id = bugs.bug_id AND actcheck.bug_when >= '2012-01-01 00:00:00' AND actcheck.bug_when <= '2012-01-02 00:00:00' AND actcheck.added = 'RESOLVED' AND actcheck.fieldid IN (29) ) 
...
WHERE ((actcheck.bug_when IS NOT NULL)) ...

Which does look more like it
Attached patch proposed fix for bugzilla-4.2.2 (obsolete) — Splinter Review
An attempt to fix this by reducing the joins to one per field activity.

Done by reordering the filters rules and changing the custom operators to complete the last join "extra" filters instead of creating a new join for each call.
(In reply to Benoit Plessis from comment #1)
> Created attachment 655247 [details] [diff] [review]
> proposed fix for bugzilla-4.2.2

however i didn't found how to reduce the number of ".bug_when IS NOT NULL" in the were so maybe there is a better way
If the fix is simple and doesn't regress anything, it can go into 4.2, else I will retarget it to 4.4. glob, could you look at this patch as you are already working on bug 780820?
Blocks: 780820
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → Bugzilla 4.2
Attachment #655247 - Flags: review?(glob)
Comment on attachment 655247 [details] [diff] [review]
proposed fix for bugzilla-4.2.2

this sort of hidden behavour existed in 4.0 and was removed on purpose.
bug 780820 is the right way to address this.
Attachment #655247 - Flags: review?(glob) → review-
actually bug 677757 is the bug i was thinking of.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Target Milestone: Bugzilla 4.2 → ---
(In reply to Byron Jones ‹:glob› from comment #5)
> actually bug 677757 is the bug i was thinking of.

This is not a duplicate of 677757, though fixing it may help fixing this one. The expected behavior of the "Search By Change History" section is unambiguous and it should use one single table join matching all the conditions set in this section.
No longer blocks: 780820
Status: RESOLVED → REOPENED
Depends on: 677757
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Flags: blocking4.4+
Keywords: regression
I to ran across this.  This was my use case:

"Show me all of the bugs whose Status was set to Resolved between 4 weeks ago and 3 weeks ago".

What it actually delivered was all of the bugs that were ever set to Resolved, and who's delta_ts was between 4 weeks ago and 3 weeks ago.

delta_ts cannot be part of the date range query because if a bug gets changed later it won't meet the search criteria effectively.  It really needs to go on bug_when in the activity log.

Also, the field change AND the date start AND the date end need to be combined into a single statement in the query, otherwise it matches them separately and the results are not what is expected.

This worked throughout 3.4, it seems to be broken this way throughout the 4.2.x release.

I came up with a total-hack fix that corrected the problem for me.  I do not know if it causes other problems.  All of the fixes go into Search.pm.

In _special_parse_chfield, comment out the two statements that add delta_ts to the clause.  It's these two statements:

#    if ($date_from ne '') {
#        $clause->add('delta_ts', 'greaterthaneq', $date_from);
#    }

-and-

#    if ($date_to ne '' and !@fields and $value_to eq '') {
#        $clause->add('delta_ts', 'lessthaneq', $date_to);
#    }


Then, in _changedbefore_changedafter, there's a line that says 

my $table = "act_${field_id}_$chart_id";

Change it to 

my $table = "act_${field_id}_XXX";  

(I don't know if I could have left the trailing XXX off, I was concerned I'd have duplicate join tables if I did this)

In _changedfrom_changedto, I did the exact same thing:  find the following line:

my $table = "act_${field_id}_$chart_id";

Change it to 

my $table = "act_${field_id}_XXX";  

With these changes, the SQL looks like what I'd expect for a date range query on a field change and the results are consistent.

I'm not a contributor and don't create patches so I'm just submitting what I did in hopes it'll lead to new insights.
(In reply to Joe Reifel from comment #10)
> I to ran across this.  This was my use case:
> 
> "Show me all of the bugs whose Status was set to Resolved between 4 weeks
> ago and 3 weeks ago".
> 
> What it actually delivered was all of the bugs that were ever set to
> Resolved, and who's delta_ts was between 4 weeks ago and 3 weeks ago.

We're also seeing this behavior, and with the same use case as Joe gives to reproduce, since upgrading to 4.2 (vanilla install). It's definitely not a duplicate of bug 677757, although I don't have access to make changes to the install in order to test Benoit's proposed patch or Joe's proposed fix.
My change did cause one other problem - it broke the searches where date ranges were specified but no field was specified.  For example, to look for all changes in the system in the last day.

Re-pasting what I did to fix both problems.

In _special_parse_chfield, make sure it is checking for "and !@fields" in the first statement, as follows:

    if ($date_from ne '' and !@fields ) {
        $clause->add('delta_ts', 'greaterthaneq', $date_from);
    }


Then, in _changedbefore_changedafter, there's a line that says 

my $table = "act_${field_id}_$chart_id";

Change it to 

my $table = "act_${field_id}_XXX";  

(I don't know if I could have left the trailing XXX off, I was concerned I'd have duplicate join tables if I did this)

In _changedfrom_changedto, I did the exact same thing:  find the following line:

my $table = "act_${field_id}_$chart_id";

Change it to 

my $table = "act_${field_id}_XXX";
Too late for 4.4. Let's see if we can have this bug fixed for 4.4.1.
Flags: blocking4.4.1+
Flags: blocking4.4-
Flags: blocking4.4+
Target Milestone: --- → Bugzilla 4.4
Attached patch patch, v1Splinter Review
We need to use Bugzilla::Search::ClauseGroup to group conditions together. If several fields are selected, we loop over them using a OR condition (because the UI says "ANY of the fields").

I checked results by requesting the DB directly and I found no problem.
Assignee: query-and-buglist → LpSolit
Attachment #655247 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #799686 - Flags: review?(glob)
Attachment #799686 - Flags: review?(dkl)
No longer depends on: 677757
Comment on attachment 799686 [details] [diff] [review]
patch, v1

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

r=glob
Attachment #799686 - Flags: review?(glob) → review+
Attachment #799686 - Flags: review?(dkl)
Flags: approval?
Flags: approval4.4?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search.pm
Committed revision 8736.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified Bugzilla/Search.pm
Committed revision 8606.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: