274640 - Custom sort cookies which include LIMIT are now forbidden; scary error appears

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Dave Miller [:justdave]]

We have a FREQUENTLY ocurring ThrowCodeError invocation which I've now received
several copies of on the bugzilla-admin alias, which gives the error:

The custom sort order specified in your cookie contains an invalid column name
/relevance DESC LIMIT 200/. The cookie has been cleared.

And asks the user to mail it to the admin.

This kind of thing shouldn't even be throwing an error.  This should put up a
little green box at the top of the query results saying it's using a default
sort order because their sort order stored in their cookie was invalid, and run
the query anyway.

Comment 1

[Jack Tanner]

Is 274423 a dupe of this?

Comment 2

[Jack Tanner]

Is bug 274423 a dupe of this?

Comment 3

[Dave Miller [:justdave]]

sort of, but not quite...  see my comment there about this bug.

Comment 4

[Dave Miller [:justdave]]

*** Bug 274423 has been marked as a duplicate of this bug. ***

Comment 5

[Dave Miller [:justdave]]

Comment from bug 274423:

OK, I found the cause of this...

The checkin from bug 237176 removed the following chunk of code from buglist.cgi:

-# Even more disgusting hack: if we are doing a full text search,
-# order by relevance instead of anything else, and limit to 200 results.
-if ($search->{'sorted_by_relevance'}) {
-    $db_order = $order = "relevance DESC LIMIT 200";
-    $vars->{'sorted_by_relevance'} = 1;
-}

Note that the code that *used* to be there added the "relevance DESC LIMIT 200"
to the sort order variable instead of tacking it directly onto the query itself
(which is what the current code does).  This would have resulted in a cookie
getting set with that as their sort order once the page is displayed.  When this
was removed, the checks to insure a valid sort order were also beefed up.  That
chunk no longer passes the test, because "LIMIT 200" isn't a legal part of the
sort order (and never was - that's a different portion of the SQL query).

So the people who are hitting this are people who have made use of the Specific
search prior to our upgrade, and are getting this on the first query they make
after using Bugzilla for the first time after the upgrade.

There is, then, nothing we can really do to prevent this from happening, and the
only thing we need to do is deal with it in a less threatening way.

Comment 6

[Dave Miller [:justdave]]

Bug 163114 seems to be the one responsible for turning this into a ThrowCodeError.

Comment 7

[Gervase Markham [:gerv]]

Given that this was our mistake, should we instead add code to check for this
particular condition and silently fix things up?

Gerv

Comment 8

[Dave Miller [:justdave]]

*** Bug 275602 has been marked as a duplicate of this bug. ***

Comment 9

[Kevin Benton]

I agree with Gerv - fixing this up silently is the best way to handle it.  The
impression I got at first blush was based on the error message I got.  To quote
Bugzilla:

<tt>
  <p>Bugzilla has suffered an internal error. Please save this page and send
    it to [e-mail] with details of what you were doing at
    the time this message appeared.
  </p>
  <script type="text/javascript"> <!--
    document.write("<p>URL: " + document.location + "</p>");
  // -->
  </script>
</tt>

That says that the error was severe enough that an administrator needs to get
involved while in reality, simply hitting refresh fixes the problem.  As an
administrator, that stinks.  It will mean that I will have more work to do
because users will stop dead in their tracks and call me to report this error. 
I will need to take time to explain each time that error message comes up. 
IMNSHO (agreeing with Gerv), BZ ought to be able to fix that by itself by fixing
the cookie and displaying appropriate results as if the LIMIT had not been
specified in the cookie.

Also, please consider changing the summary for this bug.  I didn't find it when
I looked for "custom sort cookie".

Comment 10

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This one-liner should do the trick. It seems to work on my system (although I
have to edit cookies.txt manually to add "relevance DESC LIMIT 200" to the
LASTORDER cookie).

Gerv

Comment 11

[Dave Miller [:justdave]]

I've filed bug 276728 to pick up the parts of this issue that weren't resolved
with this patch.

Comment 12

[Gervase Markham [:gerv]]

Fixed on trunk and branch.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.266; previous revision: 1.265
done

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.255.2.6; previous revision: 1.255.2.5
done

Gerv