282737 - Software Error from time-tracking fields during Change Columns

Status: RESOLVED FIXED

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

Description

[Paul]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.1.4322)
Build Identifier: Bugzilla 2.18

I did a query, I went to change columns, I removed the time tracking columns 
and bam I got a software error.

Reproducible: Always

Steps to Reproduce:
1. Run a query with columns Orig Est Time & percent complete turned on
2. Press change columns
3. turn off orig est and percent complete
4. press change columns
5. BAM! death.

Actual Results:  
BAM! death.

Software error:
DBD::mysql::st execute failed: You have an error in your SQL syntax.  Check the 
manual that corresponds to your MySQL server version for the right syntax to 
use near '.bug_idSUMldtime.work_timeCOUNTDISTINCTldtime.bug_whenCOUNTbugs [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.realname, bugs.bug_status, bugs.resolution, 
bugs.short_desc, bugs.estimated_time, 
100SUMldtime.work_timeCOUNTDISTINCTldtime.bug_whenCOUNTbugs.bug_idSUMldtime.work
_timeCOUNTDISTINCTldtime.bug_whenCOUNTbugs.bug_idbugs.remaining_timeASpercentage
_complete FROM bugs, profiles AS map_assigned_to  LEFT JOIN bug_group_map  ON 
bug_group_map.bug_id = bugs.bug_id  AND bug_group_map.group_id NOT IN 
(12,3,8,4,13,1,6,9,11,2,5,10,7)  LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND 
cc.who = 1 WHERE bugs.assigned_to = map_assigned_to.userid AND (bugs.bug_id IN 
('1304','1001','1155','920','1037','1083','1161','1165','1169','1172','1295','13
11','855','1101','1250','940','1303','514','1282','1305','1306','1087','438','64
4','799','800','1128','1207','1237','416','1249','452','801','1064','413','446',
'1199','867','872','1071','1012','1171','913')) AND ((bug_group_map.group_id IS 
NULL)    OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1)     OR 
(bugs.cclist_accessible = 1 AND cc.who IS NOT NULL)     OR (bugs.assigned_to = 
1) ) GROUP BY bugs.bug_id ORDER BY 
bugs.resolution,map_assigned_to.realname,bugs.estimated_time,map_assigned_to.rea
lname,bugs.resolution,map_assigned_to.realname,bugs.bug_status,percentage_comple
te,bugs.resolution,map_assigned_to.realname,bugs.resolution,bugs.estimated_time,
map_assigned_to.realname,bugs.resolution,bugs.estimated_time,bugs.resolution,map
_assigned_to.realname,bugs.estimated_time,map_assigned_to.realname,bugs.bug_seve
rity,bugs.priority,bugs.bug_severity,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/bugzilla/buglist.cgi line 
766

For help, please send mail to the webmaster ([e-mail]), giving this error 
message and the time and date of the error. 


Expected Results:  
No death.

I havn't tried this a lot, but it seems that I cannot pass by it now.

Comment 1

[Paul]

I did some more testing...

After that no queries worked for me.  I logged out and back into bugzilla to no 
avail.  I logged in as another user and I was getting the same error.

Finally I deleted my cookies and everything started working again.

Comment 2

[Max Kanat-Alexander]

That is really weird. What version of Bugzilla do you have?

The problem is obviously:

"100SUMldtime.work_timeCOUNTDISTINCTldtime.bug_whenCOUNTbugs.bug_idSUMldtime.work
_timeCOUNTDISTINCTldtime.bug_whenCOUNTbugs.bug_idbugs.remaining_timeASpercentage
_complete"

Comment 3

[Paul]

Bugzilla Version 2.18

Comment 4

[Joel Peshkin]

I'll bet that the probelm results from sorting on a column that is no longer in
the display list.  

Comment 5

[Joel Peshkin]

OK... this is 2.18 only and easy to reproduce

Comment 6

[Joel Peshkin]

oops... 2.19 also

Comment 7

[Joel Peshkin]

Attachment: Patch v1

I'm not sure if this is the simplest way to do this.

Comment 8

[Joel Peshkin]

While the patch will work, the right fix for this will be to store the column's
name in the cookie.

Comment 9

[Frédéric Buclin]

(In reply to comment #4)
> I'll bet that the probelm results from sorting on a column that is no longer in
> the display list.  

joel, Marc and I cannot reproduce this bug using either 2.18.1 or 2.19.3. Could
you give us a more precise testcase?

Comment 10

[Joel Peshkin]

1. Run a query with columns Orig Est Time & percent complete turned on
2. Sort by percent complete
3. Press change columns
4. turn off orig est and percent complete
5. BAM! death.

Comment 11

[Marc Schumann [:Wurblzap]]

Comment on attachment 178085 [details] [diff] [review]
Patch v1

This doesn't fix it for me...

1. Run a query with column percent complete turned on
2. Sort by percent complete
3. Press change columns
4. turn off percent complete
5. BAM! death: "Unknown table 'ldtime' in field list"

Comment 12

[Colin Ogilvie [:cso]]

Attachment: 2.20 Branch Patch v2

This seems to fix the issue with the ldtime and also the original issue...

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 189304 [details] [diff] [review]
2.20 Branch Patch v2

This fixes the issue at hand.

>         # Add order columns to selectnames
>         # The fragment has already been validated
>         $fragment =~ s/\s+(asc|desc)$//;
>-        $fragment =~ tr/a-zA-Z\.0-9\-_//cd;
>+        if ($fragment =~ / AS (\w+)/)
>+        {
>+            $fragment = $columns->{$1}->{'name'};
>+        }
>+        else
>+        {
>+            $fragment =~ tr/a-zA-Z\.0-9\-_//cd;
>+        }
>         push @selectnames, $fragment;

If you think it's not only me having troubles grasping this, then please
code-comment what's being done here... ;)

>-    if (lsearch($fieldsref, "(SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) AS actual_time") != -1) {
>+    if (lsearch($fieldsref, "(SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id)) AS actual_time") != -1 ||
>+        lsearch($fieldsref, "(100*((SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id))/" .
>+                            "((SUM(ldtime.work_time)*COUNT(DISTINCT ldtime.bug_when)/COUNT(bugs.bug_id))".
>+                            "+bugs.remaining_time))) AS percentage_complete") != -1) {
>         push(@supptables, "INNER JOIN longdescs AS ldtime " .
>                           "ON ldtime.bug_id = bugs.bug_id");
>     }

This is evil imho. If I get it right, these two strings correspond to identical
strings in buglist.cgi. These two strings should be converted into Search.pm
constants. Doing so, we can avoid introducing regressions if these need a
change again. Plus, the DefineColumn block in buglist.cgi and the
lsearch($fieldsref)+push(@supptables) block in Search.pm would get a visible
interconnection.

On a related nit, is it possible to move the ldtime @supptables filler up to
the other similar @supptables fillers?

Comment 14

[Joel Peshkin]

I agree with comment 13, but I will not be able to do anything about this until
well after 2.20.  If someone wants to take a stab at this, I should be able to
review it but I will probably need someone else to test it.

Comment 15

[Colin Ogilvie [:cso]]

(In reply to comment #14)
> I agree with comment 13, but I will not be able to do anything about this until
> well after 2.20.  If someone wants to take a stab at this, I should be able to
> review it but I will probably need someone else to test it.

I agree with the code comment bit and perhaps with the moving bit, but disagree
with making it constants since the other definitions aren't constants (that
could be a spinoff I guess).

Comment 16

[Joel Peshkin]

Right...  not literally constants, but there are named things that don't
correspond to database fields and which Search.pm knows how to deal with.  I'm
not sure if there are such things in the display column list.  There certainly
are some that effect bug selection.

Comment 17

[Colin Ogilvie [:cso]]

Attachment: Patch v3

Patch 3...

Changes:

Add's a brief comment (doesn't describe what the code does, just explains what
it fixes) above the section of code in buglist.cgi (and, annoyingly, removes a
^M from the file too...)

Moved the section of code in Search.pm up to where the rest of the code is (the
nit)

On the duplication of SQL, I changed it to just "grep"ing for the AS
actual_time / AS percentage_complete bit of the code to make it simpler (and
remove duplication) as I can't see any easier way of doing it.

Comment 18

[Marc Schumann [:Wurblzap]]

Comment on attachment 189753 [details] [diff] [review]
Patch v3

I like your solution. Joel, do you feel inclined to brush up the comment?

Works well, r=wurblzap.

Comment 19

[Colin Ogilvie [:cso]]

Note the same patch should apply to the branch as well.

Comment 20

[Marc Schumann [:Wurblzap]]

Applies (and works) on the 2.20 branch as well.
Doesn't apply to the 2.18 branch -- Colin, can you please provide for a backport?

Comment 21

[Colin Ogilvie [:cso]]

Attachment: 2.18 Patch v1

2.18 Patch v1

Comment 22

[Marc Schumann [:Wurblzap]]

Comment on attachment 189771 [details] [diff] [review]
2.18 Patch v1

Ok, the backport works, too.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 189753 [details] [diff] [review]
Patch v3

>+        if ($fragment =~ / AS (\w+)/)
>+        {
>+            $fragment = $columns->{$1}->{'name'};
>+        }
>+        else
>+        {
>+            $fragment =~ tr/a-zA-Z\.0-9\-_//cd;
>+        }

Per Bugzilla style guide, this should be:

>+        if ($fragment =~ / AS (\w+)/) {
>+            $fragment = $columns->{$1}->{'name'};
>+        }
>+        else {
>+            $fragment =~ tr/a-zA-Z\.0-9\-_//cd;
>+        }

Please fix this before checking in and post an updated patch to the bug.

Comment 24

[Colin Ogilvie [:cso]]

Attachment: Trunk patch for checkin

Patch for Trunk, as checked in

Comment 25

[Colin Ogilvie [:cso]]

Attachment: 2.20 Patch for checkin

Comment 26

[Colin Ogilvie [:cso]]

Attachment: 2.18 patch for checkin

2.18 Patch

Comment 27

[Colin Ogilvie [:cso]]

Trunk:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.300; previous revision: 1.299
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.103; previous revision: 1.102
done

2.20:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.299.2.1; previous revision: 1.299
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.99.2.1; previous revision: 1.99
done

2.18:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.255.2.11; previous revision: 1.255.2.10
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.57.2.9; previous revision: 1.57.2.8
done