Software Error from time-tracking fields during Change Columns

RESOLVED FIXED in Bugzilla 2.18

Status

()

P2
critical
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: pjdemarco, Assigned: cso)

Tracking

2.18
Bugzilla 2.18
Bug Flags:
approval +
approval2.20 +
blocking2.20 +
approval2.18 +
blocking2.18.4 +
blocking2.18.3 -
blocking2.18.1 -

Details

(Whiteboard: [Ready for trunk][Ready for 2.20][Ready for 2.18], URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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 (xxx@ppg.com), 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.
(Reporter)

Comment 1

14 years ago
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

14 years ago
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"
(Reporter)

Comment 3

14 years ago
Bugzilla Version 2.18

Updated

14 years ago
OS: Windows 2000 → All
Hardware: PC → All
Version: unspecified → 2.18

Updated

14 years ago
Summary: Software Error: Change Columns → Software Error from time-tracking fields during Change Columns

Comment 4

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

Comment 5

14 years ago
OK... this is 2.18 only and easy to reproduce
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.18.1?
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18

Comment 6

14 years ago
oops... 2.19 also
Flags: blocking2.20?

Comment 7

14 years ago
Created attachment 178085 [details] [diff] [review]
Patch v1

I'm not sure if this is the simplest way to do this.
Assignee: query-and-buglist → bugreport
Status: NEW → ASSIGNED

Comment 8

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

Flags: blocking2.20?
Flags: blocking2.20+
Flags: blocking2.18.2+
Flags: blocking2.18.1?
Flags: blocking2.18.1-

Updated

14 years ago
Attachment #178085 - Flags: review?

Updated

14 years ago
Whiteboard: [patch awaiting review]

Comment 9

13 years ago
(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

13 years ago
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 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"
Attachment #178085 - Flags: review? → review-
Whiteboard: [patch awaiting review]
Whiteboard: [bug awaiting patch]
Flags: blocking2.18.4+
Flags: blocking2.18.3-
Flags: blocking2.18.2+
(Assignee)

Comment 12

13 years ago
Created attachment 189304 [details] [diff] [review]
2.20 Branch Patch v2

This seems to fix the issue with the ldtime and also the original issue...
Attachment #178085 - Attachment is obsolete: true
Attachment #189304 - Flags: review?(wurblzap)
(Assignee)

Updated

13 years ago
Whiteboard: [bug awaiting patch] → [patch awaiting review]
(Assignee)

Updated

13 years ago
Attachment #189304 - Attachment description: Patch v2 → 2.20 Branch Patch v2
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?
Attachment #189304 - Flags: review?(wurblzap) → review-

Comment 14

13 years ago
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.

(Assignee)

Comment 15

13 years ago
(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

13 years ago
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.
(Assignee)

Comment 17

13 years ago
Created attachment 189753 [details] [diff] [review]
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.
(Assignee)

Updated

13 years ago
Attachment #189304 - Attachment is obsolete: true
Attachment #189753 - Flags: review?(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.
Attachment #189753 - Flags: review?(wurblzap) → review+
Whiteboard: [patch awaiting review] → [ready for trunk]
(Assignee)

Comment 19

13 years ago
Note the same patch should apply to the branch as well.
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?
Whiteboard: [ready for trunk] → [Ready for trunk][Ready for 2.20][Awaiting patch for 2.18]
(Assignee)

Comment 21

13 years ago
Created attachment 189771 [details] [diff] [review]
2.18 Patch v1

2.18 Patch v1
Assignee: bugreport → colin.ogilvie
Attachment #189771 - Flags: review?(wurblzap)
Comment on attachment 189771 [details] [diff] [review]
2.18 Patch v1

Ok, the backport works, too.
Attachment #189771 - Flags: review?(wurblzap) → review+
Flags: approval?
Flags: approval2.20?
Flags: approval2.18?
Whiteboard: [Ready for trunk][Ready for 2.20][Awaiting patch for 2.18] → [Ready for trunk][Ready for 2.20][Ready for 2.18]
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.
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
(Assignee)

Comment 24

13 years ago
Created attachment 189832 [details] [diff] [review]
Trunk patch for checkin

Patch for Trunk, as checked in
Attachment #189753 - Attachment is obsolete: true
Attachment #189771 - Attachment is obsolete: true
(Assignee)

Comment 25

13 years ago
Created attachment 189833 [details] [diff] [review]
2.20 Patch for checkin
(Assignee)

Comment 26

13 years ago
Created attachment 189834 [details] [diff] [review]
2.18 patch for checkin

2.18 Patch
(Assignee)

Comment 27

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Attachment #189832 - Flags: review+
Attachment #189833 - Flags: review+
Attachment #189834 - Flags: review+
You need to log in before you can comment on or make changes to this bug.