Last Comment Bug 753688 - Classification doesn't work as z-axis on reports
: Classification doesn't work as z-axis on reports
Status: RESOLVED FIXED
: regression
Product: Bugzilla
Classification: Server Software
Component: Reporting/Charting (show other bugs)
: 4.2.1
: All All
: -- major (vote)
: Bugzilla 4.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks: 780028
  Show dependency treegraph
 
Reported: 2012-05-10 02:25 PDT by signalrauschen
Modified: 2012-08-02 16:21 PDT (History)
5 users (show)
LpSolit: approval+
LpSolit: blocking4.4+
LpSolit: approval4.2+
LpSolit: blocking4.2.2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
attachment1.jpg (101.73 KB, image/jpeg)
2012-05-10 02:25 PDT, signalrauschen
no flags Details
attachment2.jpg (36.45 KB, image/jpeg)
2012-05-10 02:26 PDT, signalrauschen
no flags Details
patch, v1 (1.95 KB, patch)
2012-07-01 10:08 PDT, Frédéric Buclin
glob: review+
Details | Diff | Splinter Review

Description signalrauschen 2012-05-10 02:25:32 PDT
Created attachment 622658 [details]
attachment1 [details] [diff] [review].jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

A report query that worked in Bugzilla 3.x does not work in Bugzilla 4.2+ any more.

I tried to generate a tabular report with the following axis parameters (see attachment 1 [details] [diff] [review]):
- Vertical axis: Status
- Horizontal axis: Severity
- Tables: Classification

The URL looks like this:
https://bugzilla.xxx.de/report.cgi?x_axis_field=bug_severity&y_axis_field=bug_status&z_axis_field=classification&query_format=report-table&short_desc_type=allwordssubstr&short_desc=&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED&longdesc_type=allwordssubstr&longdesc=&bug_file_loc_type=allwordssubstr&bug_file_loc=&deadlinefrom=&deadlineto=&bug_id=&bug_id_type=anyexact&emailtype1=substring&email1=&emailtype2=substring&email2=&emailtype3=substring&email3=&chfieldvalue=&chfieldfrom=&chfieldto=Now&j_top=AND&f1=noop&o1=noop&v1=&format=table&action=wrap


In Bugzilla 3.x this resulted in a tabular report showing a table with the following structure for each classification:

             blocker | citical | major | minor | normal | trivial | enh | Total
NEW
ASSIGNED
REOPENED
DONE
...



Actual results:

In Bugzilla 4.2+ this report query resulted in an empty page (see attachment 2 [details] [diff] [review]). No table has been generated.


Expected results:

This report query should result in a tabular report showing a table with the following structure for each classification:

             blocker | citical | major | minor | normal | trivial | enh | Total
NEW
ASSIGNED
REOPENED
DONE
...
Comment 1 signalrauschen 2012-05-10 02:26:05 PDT
Created attachment 622659 [details]
attachment2 [details] [diff] [review].jpg
Comment 2 Marc Schumann [:Wurblzap] 2012-05-10 09:22:40 PDT
I can confirm on https://landfill.bugzilla.org/bugzilla-tip/. If the classification is used as the z-axis of a report, the result is empty.
Comment 3 Frédéric Buclin 2012-05-21 11:21:28 PDT
There is something really wrong, also with other fields as z-axis. With JS turned on, all tables have the same data. With JS turned off, it seems to be fine. With the classification as z-axis, JS turned on or off doesn't help.
Comment 4 Frédéric Buclin 2012-05-21 13:01:39 PDT
The reason why reports now fail with the classification field is that Search.pm now *prepends* extra columns to the column list passed by the caller instead of appending them as it used to do in Bugzilla 4.0 and older. The consequence is that the caller no longer gets back what it passed in the 'fields' argument and data are in a pseudo-random order. AFAIK, there is no reason to prepend extra columns for the SELECT part of the SQL query. The order is only important for the FROM part.

This is a regression due to bug 577807 and/or bug 578278.
Comment 5 Frédéric Buclin 2012-07-01 10:08:45 PDT
Created attachment 638195 [details] [diff] [review]
patch, v1

AFAIK, we don't need to select columns involved in JOINs, and so _display_columns() doesn't include columns listed in %COLUMN_DEPENDS. Fields are now returned in the order defined by the caller, with extra columns at the end (which the caller can ignore).

Let me know if this regresses something.
Comment 6 Byron Jones ‹:glob› 2012-07-04 05:55:34 PDT
Comment on attachment 638195 [details] [diff] [review]
patch, v1

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

r=glob

this looks good, and i can't break it.

::: Bugzilla/Search.pm
@@ +852,5 @@
> +
> +    # Do not alter the list specified here at all, even if they are duplicated.
> +    # Those are passed by the caller, and the caller expects to get them back
> +    # in the exact same order.
> +    $self->{display_columns} = [$self->_input_columns, $self->_extra_columns];

do:

$self->{display_columns} ||= [$self->_input_columns, $self->_extra_columns];

and drop the early return

@@ +878,5 @@
>  # will go into the SELECT clause.
>  sub _sql_select {
>      my ($self) = @_;
>      my @sql_fields;
> +    foreach my $column ($self->_display_columns) {

as far as i can tell it should be safe to 'uniq' display_columns here
Comment 7 Frédéric Buclin 2012-07-24 07:04:34 PDT
(In reply to Byron Jones ‹:glob› from comment #6)
> do:
> 
> $self->{display_columns} ||= [$self->_input_columns, $self->_extra_columns];
> 
> and drop the early return

Good idea.


> as far as i can tell it should be safe to 'uniq' display_columns here

I prefer not to, as the caller may pass the same column twice.


Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search.pm
Committed revision 8296.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/Search.pm
Committed revision 8103.
Comment 8 Frédéric Buclin 2012-08-02 16:04:54 PDT
(In reply to Byron Jones ‹:glob› from comment #6)
> as far as i can tell it should be safe to 'uniq' display_columns here

I'm doing some tests with Oracle right now, and I see that Oracle crashes if the SQL query contains a column in ORDER BY which appears twice in SELECT. So for instance

  "SELECT bug_id, assigned_to, assigned_to FROM bugs ORDER BY assigned_to"

crashes in Oracle (because it complains that the assigned_to column in ORDER BY is ambiguous), but it works correctly in MySQL and PostgreSQL. If I explicitly specify the table name in ORDER BY, i.e. "ORDER BY bugs.assigned_to", then Oracle is happy.

So to fix this problem, we have 2 solutions:
a) remove duplicated columns in _display_columns, but this may confuse the caller which expects some list order. So if the caller wants "bug_id, assigned_to, assigned_to", we would return "bug_id, assigned_to" only, which is unexpected.
b) prefix all columns in ORDER BY with the table they belong to, but how do we get this information? This doesn't seem to be a viable solution, unless someone knows how to do this.

Note You need to log in before you can comment on or make changes to this bug.