Closed
Bug 491467
Opened 16 years ago
Closed 15 years ago
Every other query throws "Invalid column name" code error
Categories
(Bugzilla :: Query/Bug List, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.4
People
(Reporter: wicked, Assigned: mkanat)
Details
(Keywords: regression)
Attachments
(2 files, 8 obsolete files)
(deleted),
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wicked
:
review+
|
Details | Diff | Splinter Review |
Every other query ends with an "Invalid column name" error: The custom sort order specified in your cookie contains an invalid column name map_assigned_to.login_name. The cookie has been cleared.
This only happens when I'm not logged in. Workaround is to refresh/rerun the same query as that will magically succeed. Second refresh/rerun produces the error again so the cookie is not really cleared. Happens on both 3.4 branch and trunk but not on 3.2 branch.
I suspect this happens due to bug 219021.
Assignee | ||
Comment 2•16 years ago
|
||
Yeah, I know about this, and we should definitely fix it before 3.4.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Updated•16 years ago
|
Severity: trivial → major
Priority: -- → P1
Target Milestone: --- → Bugzilla 3.4
Updated•16 years ago
|
Flags: testcase?
Comment 3•16 years ago
|
||
I don't know if that's related, but something is wrong with
[% order = order.remove("$column.sortalias( desc)?,?") %]
in list/table.html.tmpl around line 144. Before this line, order is (in my test case) assigned_to_realname,bugs.bug_id but after this line, it's _realname,bugs.bug_id. Note that assigned_to disappeared. This happens if you try to sort consecutive times on the assignee realname.
wicked, what's your STR to get the error reported in comment 0?
Updated•16 years ago
|
Status: ASSIGNED → NEW
Reporter | ||
Comment 4•16 years ago
|
||
(In reply to comment #3)
> [% order = order.remove("$column.sortalias( desc)?,?") %]
This line indeed affects rest of column orders which is bug 470214 which will be fixed by the patch in bug 164009.
> wicked, what's your STR to get the error reported in comment 0?
I first do an Advanced Search with "Sort result by" set to either "Last Changed By" or Assignee. Afterwards every other QuickSearch fails with the mentioned error. Basicly first step uses map_assigned_to.login_name for ordering and stores it in LASTORDER cookie. This cookie is later used by the QS doing a "Reused last order" ordering but this time it's content doesn't pass the validation at around 1377 of buglist.cgi script.
Incidently, using map_assigned_to.login_name explicitly in order param always fails due to it hitting the same validation.
Looks like bug 219021 converted assigned_to (and other user name) column names to SQL fragments so they are no longer simply "map_assigned_to.login_name" as before. Most likely same problem has always affected actual_time and percentage_complete columns and other SQL fragment columns as I couldn't see any demangling code for them in the cookie validation (there are some in other places, like $db_order generation). This hasn't been such a problem since these are usually disabled and not used in the default orders and places like whine.pl script.
Ultimate solution would be to use column IDs instead of names in the order param. Columnlist param already works this way. Although, we still need to detect and convert old name based orders.
Comment 5•16 years ago
|
||
Tested successfully with MySQL 5.0.81 and PostgreSQL 8.3.7 on both Bugzilla 3.5 and 3.3.4. The logic is the following:
- adding an alias to user fields forces the template to use column IDs to sort bugs, instead of column names (which are DB + logged in/out dependent);
- I fixed the default sort order to use the assignee column ID instead of the column name, for the reason explained above;
- I fixed the regexp in the template which was generating a lot of errors as it was incorrectly truncating columns which have a common substring (such as foo and foo_realname). The regexp is now very robust and such errors cannot happen again.
There is one final point which I want to fix, but I'm going to do it in a separate bug: if the order list passed from the LASTORDER cookie or from the form contains invalid columns, they should be removed from the list and the buglist should be sorted based on the remaining valid columns rather than throwing an error. These error messages look like something terrific happened while it's just a minor thing. This is a very bad user experience IMO. So I'm going to convert these errors to normal messages displayed at the top of the page (as we do when an incorrect requestee couldn't be found when attaching a new attachment, for instance).
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #379119 -
Flags: review?(wicked)
Updated•16 years ago
|
Keywords: regression
Comment 6•16 years ago
|
||
(In reply to comment #5)
> while it's just a minor thing. This is a very bad user experience IMO. So I'm
> going to convert these errors to normal messages displayed at the top of the
> page
I filed bug 494369.
Reporter | ||
Comment 7•16 years ago
|
||
Comment on attachment 379119 [details] [diff] [review]
patch, v1
You also need to accept and convert old names from old cookies and bookmarks somewhere.
>Index: buglist.cgi
>===================================================================
>+ $name .= " AS ${id}_login_name";
This alias needs to be same as ID. See note 2 around line 659. Otherwise sorting goes by user id instead of by login name for these fields.
> # DEFAULT
>+ $order = "bugs.bug_status, bugs.priority, assigned_to, bugs.bug_id";
You forgot to change Assignee and "Last Changed" sort orders same way (both defined few lines above). Now a buglist without an explicit order after using one of these sort orders fails with same "Invalid column name" error.
>Index: template/en/default/list/table.html.tmpl
>===================================================================
>- [% order = order.remove("$column.sortalias( desc)?,?") %]
>+ [%# We have to escape $ to pass it to the parser, else it's seen as a variable. %]
>+ [% order = order.remove("$column.sortalias( desc)?(,\s*|\$)") %]
Nit: Would be nice to leave this fix to the patch in bug 164009 that also fixes this problem since it's not directly related to this bug. Well, at least for tip anyway.
Besides, this doesn't seem to fully work when first sorting by real name and then login name. Seems to think you want to go directly to DESC sorting of login_name instead of ASC.
Attachment #379119 -
Flags: review?(wicked) → review-
Comment 8•16 years ago
|
||
(In reply to comment #7)
> You also need to accept and convert old names from old cookies and bookmarks
> somewhere.
Hum, I intentionally didn't want to convert old names.
Reporter | ||
Comment 9•16 years ago
|
||
Why not?
Comment 10•16 years ago
|
||
Because my goal was to prevent the error, not to convert old names. But I can do it here, to make you happy. :-p
Comment 11•16 years ago
|
||
(In reply to comment #7)
> This alias needs to be same as ID. See note 2 around line 659. Otherwise
> sorting goes by user id instead of by login name for these fields.
If I do that, PostgreSQL crashes. Also, note 2 only mentions field ID vs column ID, not aliases (as far as I understand).
Reporter | ||
Comment 12•15 years ago
|
||
Interesting.. Why doesn't deadline, actual_time, percentage_complete, (assigned_to|reporter|qa_contact)_realname have the same problem then on PostgreSQL? These are all other columns that use AS alias AND have same ID (the key) in the columns hash.
Anyway the problem having ID and alias separate is real (I tested) as sorting goes by userid (since that's what the bugs table column contains) and not by login_name that comes via the JOIN.
Reporter | ||
Comment 13•15 years ago
|
||
Answering my own question.. It seems mentioned aliased columns work because there's no corresponding bugs table column to confuse PostgreSQL. Maybe one solution is to give these columns special ordering in Search.pm line 54? Or use assigned_to_login_name in order just like assigned_to_real_name is used?
Comment 14•15 years ago
|
||
I will let someone else play with SQL. I don't know all the internals of Search.pm well enough.
Assignee: LpSolit → query-and-buglist
Assignee | ||
Comment 15•15 years ago
|
||
Okay, I think I have to take this one, I don't think anybody else is going to do it.
Assignee: query-and-buglist → mkanat
Assignee | ||
Comment 16•15 years ago
|
||
Okay, this should fix it. We just use the ids for the orders everywhere, which seems to work just fine.
The only thing that might break is re-ordering saved searches, but that's only a very minor regression compared to the severity of this blocker, and it can easily be worked around by users if they just edit their search and save it again, I think.
Attachment #379119 -
Attachment is obsolete: true
Attachment #385284 -
Flags: review?(wicked)
Assignee | ||
Updated•15 years ago
|
Attachment #385284 -
Flags: review?(wicked)
Assignee | ||
Comment 17•15 years ago
|
||
Comment on attachment 385284 [details] [diff] [review]
v2
Oh, this patch breaks Pg. I think I can fix it, though.
Assignee | ||
Comment 18•15 years ago
|
||
Hey wicked. So, in order to make this all work on Pg, I actually had to do some refactoring--some stuff that I think you were going to do in another bug anyway, where I moved column stuff out of buglist.cgi and into Search.pm. I also refactored it while I was at it, because a little cleanup was a good thing, there. :-)
I handled the entire problem by making Search.pm accept column ids instead of SQL for both the columnlist and the order parameter. (This fixes all possible problems we could have been having with map_assigned_to.login_name--there were other problems if you logged out and logged back in again.)
The only problem with this is that, unfortunately, the current version of this patch does not accept the old columnlist or order values. I suspect that if I leave things this way, the world will explode and I will be stoned to death. :-) I have a solution (which is to create a hash that maps the old names to the new names) which will be coming in a future version of this patch. But I wanted to give you this version (which is reviewable), so that you could see the evolution of things, instead of just giving you a single gigantic complex patch.
Another problem with this is that it will require extensive field testing, because it's a change to the internals of Search.pm, which sometimes has hidden voodoo.
This is all something we wanted to do eventually anyway, though, so we're just doing it early. Hopefully we'll catch any problems with QA or when we release 3.4rc1, but if we don't, I suspect deploying on bugzilla.mozilla.org will catch the rest. (That might happen after the final release of 3.4, though.)
Attachment #385284 -
Attachment is obsolete: true
Attachment #385328 -
Flags: review?(wicked)
Assignee | ||
Comment 19•15 years ago
|
||
Oh, also, I took away the magic of the realname column. It just will be empty now, if the user doesn't have a realname, like it is in all the other formats of the buglist.
Assignee | ||
Comment 20•15 years ago
|
||
Okay, and here it is with the code to translate old SQL fragments into column ids. As far as I can tell, this was only needed for the order stuff, not for columnlist, since it seems the "columnlist" parameter on buglist.cgi has always used column ids.
Anyhow, this enormously simplifies both buglist.cgi and Search.pm's query-building code, and probably handles other long-standing bugs that we've otherwise had trouble dealing with.
Attachment #385328 -
Attachment is obsolete: true
Attachment #385341 -
Flags: review?(wicked)
Attachment #385328 -
Flags: review?(wicked)
Reporter | ||
Comment 21•15 years ago
|
||
Comment on attachment 385341 [details] [diff] [review]
v4
Oh, oh, OH! This is awesome! Next I just to have look what other bugs you ended up fixing (or helped fixing) with this. Thanks, mkanat, for doing this! :)
Alas, you forgot to update Search field definitions in whine.pl around line 433.
Additionally, this patch breaks fulltext searches for some reason.
(In reply to comment #19)
> Oh, also, I took away the magic of the realname column. It just will be empty
I'm not sure this is a good idea since I'm sure users want the ease of use when it comes to email/realname fields. When the magic is there, people get away with one column so they will save screen space. If this is removed they need to have two fields and one of the could always (or usually) be empty, which wastes space.
I'm not going to hold review on this, though, if approvers (yes, plural used on purpose) agree that this is change should be made.
>Index: buglist.cgi
>===================================================================
>+my $columns = Bugzilla::Search::COLUMNS;
Nit: Maybe you should remove this line and simply use Bugzilla::Search::COLUMNS from the few places it's used anymore.
> /^Last Changed$/ && do {
>+ $order = "delta_ts,bug_status,priority,assigned_to,bug_id";
This will produce invalid SQL since delta_ts is not a valid field so it can't be selected (tries to select ", AS delta_ts" which obviously is not going to work). This is because that field is named changeddate at the moment.
>+ $fragment =~ /^(.*)(\s+(?:asc|desc))?$/i;
Shouldn't that (.*) part in the regexp be non-greedy?
>+ if (exists Bugzilla::Search::COLUMNS->{$column_name}) {
Nit: This can use the already existing $columns unless you opt to remove that field.
>@@ -950,58 +864,13 @@
>-$db_order = $order; # Copy $order into $db_order for use with SQL query
Please, also remove $db_order definition at around line 804.
>+my @orderstrings = split(/,\s*/, $order);
Nit: Fun, first we join order strings above (the custom order case) and then we split it here again. We should probably make buglist create the order as an array and simply give that to Search.pm like it expects. This can be a separate bug, though.
>Index: duplicates.cgi
>===================================================================
>@@ -199,14 +199,14 @@
>- my $query = new Bugzilla::Search('fields' => [qw(bugs.bug_id
>- map_components.name
>- bugs.bug_severity
>- bugs.op_sys
>- bugs.target_milestone
>- bugs.short_desc
>- bugs.bug_status
>- bugs.resolution
Interesting, this and report.cgi indicates one could have used SQL as field names for Search.pm and it would have worked. Wonder if we should still convert old column names and not only order names.. Hmm, buglist might not have allowed this (or the change which moved away failed to address compatibility) so this might only change internal Search.pm API.. Should we relnote this if we do change it?
>Index: report.cgi
>===================================================================
>-my @selectnames = map($columns{$_}, @axis_fields);
Nit: There's now unused SQL in the columns variable above. Should they be removed with this patch or left for a separate "cleanup columns to use Search.pm definition" bug? They could be confusing to whoever reads report.cgi code if nothing else..
Also, generating a report now crashes at DB level since it tries to fetch a special "nothing" field that has blank ID.
>Index: Bugzilla/Search.pm
>===================================================================
>- my $fieldsref = $self->{'fields'};
>+ my @fields = @{ $self->{'fields'} || [] };
Oh, nice cleanup. Should probably do the same for order param (here or somewhere).
>+ # All items that are in the ORDER BY must be in the SELECT.
>+ foreach my $orderitem (@inputorder) {
>+ if (!grep($_ eq $orderitem, @fields)) {
>+ push(@fields, $orderitem);
>+ }
>+ }
You forgot that order fields can contain asc|desc so this fails (adds funky fields to SQL so DB spits it out). It's still nice that you moved this check to Search.pm, though. :)
>@@ -803,7 +898,8 @@
>+ my @sql_fields = map { COLUMNS->{$_}->{name} . ' AS ' . $_ } @fields;
>+ my $query = "SELECT " . join(', ', @sql_fields) .
I don't think it's good idea to give every field an AS alias. This changes the SQL to use syntax like "bugs.bug_id AS bug_id, bugs.bug_severity AS bug_severity" which is awful. Also, won't this bring some problems for some DBs since we now have both bug_status alias and table? MySQL and Pg seems to work but how about Oracle?
Is there any valid reason we should be doing this? Other than code simplicity?
Also, the first line sometimes produces a "Use of uninitialized value in concatenation (.) or string" error.
>@@ -831,17 +927,12 @@
>+ # These fields are aggregates and never go into the ORDER BY.
Should that comment say GROUP BY? Also, technically bug_id wouldn't fit to this comment since I don't think it's an "aggregate". ;)
>+ my $col = COLUMNS->{$field}->{name};
>+ push(@groupby, $col) if !grep($_ eq $col, @groupby);
This line seems to cause multiple "Use of uninitialized value in string eq" errors to be logged.
> $query .= ") " . $dbh->sql_group_by("bugs.bug_id", join(', ', @groupby));
And this line sometimes logs a "Use of uninitialized value in join or string" error.
>@@ -1052,6 +1143,30 @@
>+ if ($column =~ /AS\s+(\w+)$/i) {
>+ return $1;
>+ }
Wouldn't that regexp need a blank before AS just to be safe?
>+ # product, component, classification, assigned_to, qa_contact, reporter
>+ elsif ($column =~ /map_(\w+)s?\.(login_)?name/i) {
>+ return $1;
>+ }
This doesn't seem to fully work.. At least order=map_products.name says it's invalid but for some reason map_assigned_to.realname (as well as .login_name) does work. :/
>+ # If it doesn't match the regexps above, check to see if the old
>+ # SQL fragment matches the SQL of an existing column
>+ foreach my $key (%{ COLUMNS() }) {
>+ return $key if COLUMNS->{$key}->{name} eq $column;
>+ }
What case should this handle? Maybe this is the reason map_assigned_to.realname works.. But most other SQL fragmented goes via the AS alias case above.
>Index: template/en/default/list/table.html.tmpl
>===================================================================
Can't fully test this since desc ordering is broken. But shouldn't you use the robust SQL fixes from the LpSolit's first patch to this bug?
Attachment #385341 -
Flags: review?(wicked) → review-
Comment 22•15 years ago
|
||
(In reply to comment #21)
> > Oh, also, I took away the magic of the realname column. It just will be empty
>
> I'm not sure this is a good idea since I'm sure users want the ease of use when
> it comes to email/realname fields. When the magic is there, people get away
> with one column so they will save screen space. If this is removed they need to
> have two fields and one of the could always (or usually) be empty, which wastes
> space.
I agree with wicked. I only display realnames, and there are many users with no realname, so I'm happy to see login names being displayed instead.
Assignee | ||
Comment 23•15 years ago
|
||
Hey hey. Thanks for the review, wicked! :-) I'm fixing things now, but the points that require a response, I wanted to respond to now:
* The reason that I removed the magic of the realname column is that there's
no way to know inside of COLUMNS, now, what buglist format we're asking for.
Probably the right way to restore this behavior is to make sure that, for
example, assigned_to is always in @fields if assigned_to_realname is there,
and then, in the template, display the login_name if the realname is empty.
* I will relnote the Search.pm API change for sure, but there's no need to
translate the columnlist SQL. I actually tried doing that and it just caused
a lot of trouble.
* We should leave the report.cgi cleanup for another bug. Because it does
validation of what can be reported on, it could have security implications,
and I don't want to worry about that right now.
* The "giving every field an AS alias" is actually the *entire point* of the
second and third versions I posted of the patch. :-) It's because Pg can't
understand what we're asking for in the ORDER BY clause, otherwise. This
also makes our SELECT totally consistent, and also some day will allow us
to use selectall_arrayref with a {Slice=>{}} argument to create Bug objects
out of the Search.pm results.
> What case should this handle?
All the items that didn't have an AS, like "bugs.bug_id". Also, any other crazy SQL that we might have had at any time in any former Bugzilla version--I'd basically rather just be safe than sorry.
Keywords: relnote
Whiteboard: [relnote Search.pm API change]
Assignee | ||
Comment 24•15 years ago
|
||
And as far as the template regexps, I don't see any problems or bugs with them, so I think I'll keep them as I had them. If you find a problem with them, let me know and I will fix them.
Assignee | ||
Comment 25•15 years ago
|
||
Okay, this addresses everything, including the realname issue. There also shouldn't be any more undef warnings, but if there are, please let me know.
Attachment #385341 -
Attachment is obsolete: true
Attachment #386791 -
Flags: review?(wicked)
Comment 26•15 years ago
|
||
(In reply to comment #24)
> And as far as the template regexps, I don't see any problems or bugs with them,
> so I think I'll keep them as I had them. If you find a problem with them, let
> me know and I will fix them.
I explained in comment 5 why I fixed the regexp:
"I fixed the regexp in the template which was generating a lot of errors as it
was incorrectly truncating columns which have a common substring (such as foo
and foo_realname). The regexp is now very robust and such errors cannot happen
again."
Assignee | ||
Comment 27•15 years ago
|
||
I fixed two problems:
1) On Pg, items in @orderby with an ASC or DESC on them weren't being correctly put into the GROUP BY.
2) On Pg, the relevance item needed to be in the GROUP BY or Pg would throw an error.
I also re-worked how the "relevance" SQL is created, and it now fixes the problem with multiple fulltext charts on Pg!! So we can fix that "quicksearch should use fulltext" bug now for 3.6. :-)
Attachment #386791 -
Attachment is obsolete: true
Attachment #386815 -
Flags: review?(wicked)
Attachment #386791 -
Flags: review?(wicked)
Reporter | ||
Updated•15 years ago
|
Attachment #386815 -
Flags: review?(wicked) → review-
Reporter | ||
Comment 28•15 years ago
|
||
Comment on attachment 386815 [details] [diff] [review]
v6
>Index: buglist.cgi
>===================================================================
>+ $direction = " desc";
...
>+ $direction = " $direction" if $direction;
Nit: Uppercase the DESC and unless I'm reading that wrong, there's no need to have blank in front of the first assignment.
>Index: report.cgi
>===================================================================
Graphical/tabular reporting still doesn't work, it generates invalid SQL that contains "...AS FROM...".
>Index: whine.pl
>===================================================================
>@@ -431,16 +431,16 @@
>+ my @searchfields = qw(
>+ bug_id,
That comma makes the compile warn and thus tests fail so please don't use it.
>Index: Bugzilla/Search.pm
>===================================================================
>@@ -52,16 +56,120 @@
> use constant SPECIAL_ORDER_JOIN => {
>- 'bugs.target_milestone' => 'LEFT JOIN milestones AS ms_order ON ms_order.value = bugs.target_milestone AND ms_order.product_id = bugs.product_id',
>+ 'target_milestone' =>
>+ 'LEFT JOIN milestones AS ms_order
>+ ON ms_order.value = bugs.target_milestone
>+ AND ms_order.product_id = bugs.product_id',
This change broke sorting on milestones due to funky regexps when handling @supptables around Search.pm line 889. It can't handle the extra newlines so we end up with "..LEFT ON ().." join which is not going to fly very far with DB query parsers.
>@@ -831,17 +937,15 @@
> foreach my $field (@fields, @orderby) {
>+ my $column_name = split_order_term($field);
...
>+ my $col = COLUMNS->{$column_name}->{name};
>+ push(@groupby, $col) if !grep($_ eq $col, @groupby);
This still produces many uninitialized value in string eq warnings. One reason for this is because @orderby contains sortkey/value pairs for select fields like priority and bug_status. These obviously aren't valid columns. Any other column with special_sql will do the same. This actually crashes on Pg since @groupby will end up having a blank entry and thus there's a dangling comma that Pg chokes on.
Do we need to check @orderby array at all any more? Did we already made sure @fields always contains @orderby contents?
>@@ -1023,9 +1127,7 @@
>+ my ($orderfield, $orderdirection) = split_order_term($orderitem);
Nit: Since this means ASC|DESC will now sometimes be in lower case it would be nice to upper case it later in this sub when the SQL order string is generated since it's a SQL term.
>@@ -1052,6 +1154,40 @@
>+ if ($column =~ /\bAS\s+(\w+)$/i) {
Now this won't match any SQL like "...) AS xx" (note the parenthesis before the AS) that's used on all time summary SQL (so now we don't accept deadline SQL order that might be present in some old saved searches).
>Index: template/en/default/list/table.html.tmpl
>===================================================================
>+ [% order = order.remove("$id( desc)?,?") %]
With the "template regexp" I was referring to this since this causes bug 470214. The patch in bug 164009 is currently going to fix that issue in addition to LpSolit's first patch in this bug.
I'll let you decide if you want to fix it here but I can also handle it in bug 470214 if you want. In any case, we need fix this issue since it breaks sorting. Unfortunately, bug 164009 is going to 3.6 only so we need to do something about 3.4 anyway.
Assignee | ||
Comment 29•15 years ago
|
||
The AS regexp works quite well. (I just tested the case you were talking about.) Are you familiar with the \b zero-width assertion in Perl regexps? (See the Assertions section of http://perldoc.perl.org/perlre.html#Regular-Expressions )
I've fixed everything else and will be uploading a new patch soon.
Assignee | ||
Comment 30•15 years ago
|
||
Okay, I ended up cleaning up report.cgi's %columns variable totally, like we had talked about, as part of fixing the report.cgi problem.
I didn't fix the order regexp--that's something we should do in a separate patch.
We might be adding more things than necessary to the GROUP BY, but I think that's okay for now.
Attachment #386815 -
Attachment is obsolete: true
Attachment #386911 -
Flags: review?(wicked)
Reporter | ||
Comment 31•15 years ago
|
||
(In reply to comment #29)
> The AS regexp works quite well. (I just tested the case you were talking
> about.) Are you familiar with the \b zero-width assertion in Perl regexps? (See
I did read the docs about \b during my review and it seems it requires a \w to be either side of the blank (hmm, blank or \s?) which I don't think matches a )-char. I tried with "order=DATE_FORMAT(bugs.deadline, '%Y-%m-%d') AS deadline" but it didn't work (said it was invalid). I think it worked in a previous version of this patch. I'm not sure if that even should work (or if it ever has worked) or if I'm just been a timeless here.. ;)
Assignee | ||
Comment 32•15 years ago
|
||
I just checked, and that particular order SQL fails because it has a comma in it--it never would have worked, because buglist.cgi treats commas as order separators. Try an expression without a comma.
Assignee | ||
Comment 33•15 years ago
|
||
wicked: See my last comment.
Reporter | ||
Updated•15 years ago
|
Attachment #386911 -
Flags: review?(wicked) → review+
Reporter | ||
Comment 34•15 years ago
|
||
Comment on attachment 386911 [details] [diff] [review]
v7
Looks like this is now working on tip for all the tests I could think of. I think we should get this is even though it's still possible there are some problems lurking around. (Personally, I fear we will end up trimming aliases and/or group by eventually.) Hopefully we catch them during QA and other tests.
This is going to need a backport to 3.4 since last two hunks of buglist.cgi changes fail to apply.
(In reply to comment #32)
> I just checked, and that particular order SQL fails because it has a comma in
Ah, yeah. Other SQL fragments do work so it's only the deadline.
(In reply to comment #33)
> wicked: See my last comment.
No need for a CC, since I reported this bug. :)
>Index: Bugzilla/Search.pm
>===================================================================
>@@ -830,17 +940,22 @@
>+ foreach my $field (@fields) {
>+ my $column_name = split_order_term($field);
Nit: This split is no longer required since these are fields and not order items.
>+ # And all items from ORDER BY must by in the GROUP BY. The above loop
Nit: s/by/be/
Assignee | ||
Comment 35•15 years ago
|
||
Okay, holding 3.4 approval for a backport, I guess.
Flags: approval3.4?
Flags: approval+
Assignee | ||
Comment 36•15 years ago
|
||
Okay, here's v7 with nits fixed, so I can backport it.
Attachment #386911 -
Attachment is obsolete: true
Attachment #387213 -
Flags: review+
Assignee | ||
Comment 37•15 years ago
|
||
Made a slight error when doing the "checkin fix" for v7.1.
Attachment #387213 -
Attachment is obsolete: true
Attachment #387217 -
Flags: review+
Assignee | ||
Comment 38•15 years ago
|
||
Okay, and here's a 3.4 backport. It's basically identical to the HEAD version, just with some conflicts resolved.
Attachment #387218 -
Flags: review?(wicked)
Reporter | ||
Comment 39•15 years ago
|
||
Comment on attachment 387218 [details] [diff] [review]
v7.2 - 3.4
>Index: buglist.cgi
>===================================================================
>@@ -898,39 +814,47 @@
>- my @order;
..
>+ my (@order, @invalid_fragments);
No need to add that @invalid_fragments. Fix that and we can move on to next level of testing by committing these changes. :)
Attachment #387218 -
Flags: review?(wicked) → review+
Assignee | ||
Updated•15 years ago
|
Flags: approval3.4? → approval3.4+
Assignee | ||
Comment 40•15 years ago
|
||
tip:
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi
new revision: 1.401; previous revision: 1.400
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl
new revision: 1.69; previous revision: 1.68
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v <-- duplicates.cgi
new revision: 1.63; previous revision: 1.62
done
Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v <-- report.cgi
new revision: 1.45; previous revision: 1.44
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v <-- whine.pl
new revision: 1.40; previous revision: 1.39
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm
new revision: 1.125; previous revision: 1.124
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm
new revision: 1.174; previous revision: 1.173
done
Checking in Bugzilla/DB/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v <-- Oracle.pm
new revision: 1.24; previous revision: 1.23
done
Checking in template/en/default/list/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v <-- table.html.tmpl
new revision: 1.44; previous revision: 1.43
done
3.4:
Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi
new revision: 1.394.2.4; previous revision: 1.394.2.3
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl
new revision: 1.68.2.1; previous revision: 1.68
done
Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v <-- duplicates.cgi
new revision: 1.62.2.1; previous revision: 1.62
done
Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v <-- report.cgi
new revision: 1.44.2.1; previous revision: 1.44
done
Checking in whine.pl;
/cvsroot/mozilla/webtools/bugzilla/whine.pl,v <-- whine.pl
new revision: 1.38.2.1; previous revision: 1.38
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v <-- DB.pm
new revision: 1.124.2.1; previous revision: 1.124
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm
new revision: 1.173.2.1; previous revision: 1.173
done
Checking in Bugzilla/DB/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v <-- Oracle.pm
new revision: 1.23.2.1; previous revision: 1.23
done
Checking in template/en/default/list/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v <-- table.html.tmpl
new revision: 1.42.2.1; previous revision: 1.42
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 41•15 years ago
|
||
Added to release notes in Bugzilla 3.4rc1.
Keywords: relnote
Whiteboard: [relnote Search.pm API change]
You need to log in
before you can comment on or make changes to this bug.
Description
•