204592 - invalid column name error (2.17.3 > 2.17.4)

Status: VERIFIED FIXED

(Bugzilla :: Query/Bug List, defect, P1)

Description

[TaeYoung Oh]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030505 Camino/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030505 Camino/0.7+

I've been evaluating as of 2.17.3. Recently upgraded to 2.17.4 via cvs update.

I got following error, when I try new query or preset queries.

//
The custom sort order specified in your cookie contains an invalid column name
map_assigned_to.login_name%2Cbugs.bug_status%2C%20bugs.priority%2C%20map_assigned_to.login_name%2C%20bugs.bug_id.
The cookie has been cleared. 
//

Strangely, this happens on Safari & Camino under Mac OS X.

Any idea?

Reproducible: Always

Steps to Reproduce:
1. Upgrade existing 2.17.3 installation (2.17.3 db&data included) via cvs update
2. Throw a query (using Safari/Camino/IE5.2/Mozilla1.3 on OSX)


Actual Results:  
The following error

//The custom sort order specified in your cookie contains an invalid column name
map_assigned_to.login_name%2Cbugs.bug_status%2C%20bugs.priority%2C%20map_assigned_to.login_name%2C%20bugs.bug_id.
The cookie has been cleared.//

I got this all the time.

Expected Results:  
Safari/Camino/IE5.2/Mozilla1.3 on OSX should show same result.
Query on Safari/Camino failed, bug succeeded on IE/Mozilla.

Comment 1

[TaeYoung Oh]

Never seen this on 2.17.3.
I tried 2.17.4 tarball and latest cvs tip.

Comment 2

[TaeYoung Oh]

The symptom is the same with all Mac-browsers.
It occurs on IE5, Mozilla1.3, Camino and Safari b2 under Mac OS X.

Comment 3

[Bradley Baetz (:bbaetz)]

Mine. Bleh.

This is fallout from CGI.pm conversion, and the backwards compat code.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: don't double quote

CGI.pm quotes for us.

Comment 5

[Bradley Baetz (:bbaetz)]

Comment on attachment 122648 [details] [diff] [review]
don't double quote

myk, can you look at this one? Quick and obvious.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 122648 [details] [diff] [review]
don't double quote

This looks right, but it doesn't work for me.  After I apply the patch on my
tip install I still get the same error:

The custom sort order specified in your cookie contains an invalid column name
bugs.delta_ts%2C%20bugs.bug_status%2C%20bugs.priority%2C%20map_assigned_to.logi
n_name%2C%20bugs.bug_id. The cookie has been cleared.

Comment 7

[Bradley Baetz (:bbaetz)]

myk: try again. Its the value in the browser which is wrong and the code
correctly complains about it.

So apply this, get the cookie cleared, then try again. You may need to select
'importance' from the query.cgi page to get a multi-valued entry in there, or
something.

Comment 8

[Bradley Baetz (:bbaetz)]

myk said that that didn't help him, but it fixed it for me. Myk, can you grab
the headers being sent, and see what cookie stuff is actually being set?

Comment 9

[Jean-Sebastien Guay]

The patch worked for me. Good work Bradley.

As an aside, I think that if the error message states :

"The custom sort order specified in your cookie contains an invalid column name 
bugs.bug_severity%2Cbugs.priority. The cookie has been cleared."

then the cookie should _really_ be cleared... Which it was not for me, and 
apparently was not for Myk either. I had to wipe my Bugzilla cookie manually 
before the patch would make any difference.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: fix clearing cookies too

If theres no -value argument, then CGI.pm will try to fetch the cookie, not
send it.

Since we already have a separate send_cookie function, use that instead, and
move the path setting stuff down into it, removing the need to override
->cookie at all.

(Note that if you have a bogus LASTORDER cookie set for /, then we never clear
it, since we send /bugzilla/, or whatever the param is. Thats nt an artifact of
this patch, though)

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 122746 [details] [diff] [review]
fix clearing cookies too

myk, take 2? Still nice and simple, really.

(This latter part is only needed post-2.17.4, for anyone who wants to fix their
existing 2.17.4 install. Not that the first part applies to 2.17.4 anyway, mind
you)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 122746 [details] [diff] [review]
fix clearing cookies too

>+    # Use CGI::Cookie directly, because CGI.pm's |cookie| method gives the
>+    # current value if there isn't a -value attribute, which happens when
>+    # we're expiring an entry.

Nit: CGI.pm's |cookie| method no longer exists, so it doesn't make sense to
talk about it in a comment.

Otherwise, looks good and works, plus it uses more standard functionality
instead of rolling our own, which is good. r=myk

Comment 13

[Jean-Sebastien Guay]

Just tried the second patch on another install (first one was running on Win32,
this one is on my Linux machine at home) and it seems to work great. Including
the cookie clearing part.

Comment 14

[Bradley Baetz (:bbaetz)]

> Nit: CGI.pm's |cookie| method no longer exists, so it doesn't make sense to
> talk about it in a comment.

No, CGI.pm's cookie method exist; I removed Bugzilla::CGI's cookie method override.

Checked in.

Comment 15

[TaeYoung Oh]

The first patch worked for me.

Thanks for all your immediate response.