257813 - CSV link from new charts gives internal error if data contains more than 1000 dates

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Jon]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8

Using the new charts - add a data set to plot.  Chart it with more then 1000
days of data.  Press the CSV button from the line chart page.  It will display
the raw html for an internal error message complaining about
 "undef error - WHILE loop terminated (> 1000 iterations)"

If we have to limit the amount of data that can be displayed we should display
an error message instead of this result.

It appears the templates limit while loops to 1000 iterations.


Reproducible: Always
Steps to Reproduce:

Comment 1

[Gervase Markham [:gerv]]

The fix is to set $Template::Directive::WHILE_MAX to a larger value. In
BUGZILLA_HOME/Bugzilla/Template.pm, add the following at around line 130, around
where the some $Template::Stash:: variables are set.

$Template::Directive::WHILE_MAX = 1000000;

Gerv

Comment 2

[Jon]

I set that in the Template.pm as suggested, but it didn't seem to help.

Comment 3

[Gervase Markham [:gerv]]

That's odd.

CCing bbaetz, who knows more about Template Toolkit than me.

Gerv

Comment 4

[Bradley Baetz (:bbaetz)]

How did you set it within Bugzilla::Template ?

Its possible that the scoping of the assignment is wrong, since the current
module is called Template.

Comment 5

[Jon]

I inserted the line
$Template::Directive::WHILE_MAX = 1000000;

right after the comment that reads
# Templatization Code

Comment 6

[Bradley Baetz (:bbaetz)]

Hmm. Try adding it to the template constructor, as one of the arguments?

Comment 7

[Gervase Markham [:gerv]]

Jon: did you ever manage to resolve this?

Gerv

Comment 8

[Jon]

No, its still broken for me.

I did try to add a line like 
WHILE_MAX => 1000000,
after the line $class->new({
in Bugzilla/Template.pm

That didn't help, but maybe thats not the way to add it to the constructor as
suggested??

Comment 9

[Frédéric Buclin]

I can reproduce the problem using 2.21.2, with exactly the same error message. This makes CSV pretty useless if we are limited to 1000 data points only.

karl, could you have a look?

Comment 10

[A. Karl Kornel]

Attachment: Patch v1

OK, this has really pissed me off.  Yes!  I say that here because comment 1 was correct!  That single line can be inserted into Template.pm, but you will not see anything --unless you re-run checksetup--, so the templates can be recompiled.  I made the change Gerv specified, ran checksetup, and the error was gone.

Anyway, I have increased the iteration limit to 1,000,000.

Comment 11

[Teemu Mannermaa (:wicked)]

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

Yuck, in your subsequent patches please don't include .svn directories. :)

This certainly fixes the problem reported here and gives support for about 2600 years.

Comment 12

[Teemu Mannermaa (:wicked)]

FWIW, this also affects 2.18 branch but not 2.16 branch that doesn't have New Charts. However, do note that the current patch won't work on 2.18. This is because checksetup.pl was changed in bug 288527 to not have duplicated (ewww!) Template instantiation code in checksetup.pl but rather use Bugzilla::Template to precompile.

Comment 13

[A. Karl Kornel]

tip:
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.37; previous revision: 1.36
done

2.20:
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.26.2.1; previous revision: 1.26
done

Comment 14

[Jon]

is this going to be fixed for 2.18 too?

Comment 15

[A. Karl Kornel]

(In reply to comment #14)
> is this going to be fixed for 2.18 too?
> 

From what I understand this would require a change in checksetup, which would require a new patch.  Said patch may be more complex and risky, but I am not sure yet.

I'm willing to do it, if you can get justdave or myk to sign off on it.

Comment 16

[Dave Miller [:justdave]]

This isn't a security or dataloss issue, it's just an annoyance, so no.  But if you *want* to do a patch, you can go ahead and post one here, and we can point people here to get it if they want to apply it themselves.