On 4/6/2010 5:59 AM, Edi Weitz wrote:

Hi Ryan,

Thanks for the patch which is kind of OK but I think can be improved. I wouldn't worry too much about performance (we're talking about very short lists here), but there's no reason to write a function. Just use REMOVE-DUPLICATES, or even DELETE-DUPLICATES.

I also like the idea of giving the user a choice which could be controlled by a global special variable (and see the :FROM-END keyword argument to REMOVE-DUPLICATES). I'd probably call the variable *REMOVE-DUPLICATE-COOKIES-P* with the possible values NIL, T, :KEEP-LAST, :KEEP-FIRST where T would mean the same as :KEEP-LAST. In this case you'd have to export the name of the variable and add it to the HTML documentation.

I'm kind of unsure, though, what the best default behavior should be. Do it like Firefox does it? What do others think?

According to RFC2109 (HTTP State Management Mechanism http://www.ietf.org/rfc/rfc2109.txt), section 4.2.2 "Set-Cookie Syntax"

... If an attribute appears more than once in a cookie, the behavior is undefined.

So... no help there. I'll send out a revised patch shortly.

Thanks, Edi.

On Sat, Apr 3, 2010 at 12:20 AM, Ryan Davis ryan@acceleration.net wrote:

...

I'm running into a problem with duplicated cookies. This is due to misbehaving servers sending back duplicate Set-Cookie headers in the same response, like this:

Set-Cookie: JSESSIONID=foo; Path=/; Secure Set-Cookie: JSESSIONID=bar; Path=/; Secure

Firefox deals with this by picked the last cookie value specified. There is no logic in drakma currently to handle that, and so my cookie-jar is getting two JSESSION cookies, and further requests are sending both cookies. I do not control the servers returning me these duplicate headers.

This patch adds a unique-cookies function that iterates through cookie objects, keeping only the last one. This is then called in get-cookies to ensure no duplicate cookie objects are propagated to the rest of the system. I've got my "make it work on a friday afternoon" solution in the attached patch, but I'm not really happy with it long term, and wanted some advice. Problems I see:

I think the dolist/find combination is a recipe for performance problems with many cookies It seems odd to build up a list of cookies, then rebuild it The most common case of no duplicate cookies has an extra bunch of consing to rebuilding up the list I think there should be a user choice of "ignore all but the last cookie" vs "ignore all but the first cookie" vs "leave them all in there"

Some thoughts on how to proceed:

Add a exported *redundant-cookie-strategy* variable, with values :keep-all, :keep-first, and :keep-last, defaulting to :keep-all (the current behavior). remove the unique-cookies function and rewrite get-cookies to implement the *redundant-cookie-strategy*, making one pass through parsed-cookies. leave get-cookies mostly as-is, but add some detection for duplicates, and run unique-cookies only if it we need to

Thanks, Ryan

PS: I'm still kinda new to contributing on open source projects via emailed patches, sorry if this is in a bad format. I read http://weitz.de/patches.html and created this patch from my working copy using >diff -u ../drakma-1.1.0/cookies.lisp cookies.lisp

---

drakma-devel mailing list drakma-devel@common-lisp.net http://common-lisp.net/cgi-bin/mailman/listinfo/drakma-devel

---

drakma-devel mailing list drakma-devel@common-lisp.net http://common-lisp.net/cgi-bin/mailman/listinfo/drakma-devel

On 4/6/2010 9:43 AM, Edi Weitz wrote:

But that's about different /attributes/ in the same cookie, that's not exactly the same, is it?

Yes, that's correct. Reading further I see:

4.3.3 Cookie Management

If a user agent receives a Set-Cookie response header whose NAME is the same as a pre-existing cookie, and whose Domain and Path attribute values exactly (string) match those of a pre-existing cookie, the new cookie supersedes the old.

I think this justifies :KEEP-LAST as the default behavior, if you interpret the first Set-Cookie header creating the cookie, and the second Set-Cookie header superseding the pre-existing cookie.

I've attached a patch that introduces *remove-duplicate-cookies-p* as you mentioned in earlier email, with NIL as the default, so existing behavior is maintained. I haven't exported the symbol or added documentation yet, but wanted to run this by you first.

I wrote a few unit tests (using the LIFT framework, http://common-lisp.net/project/lift/), are you interested in those? I'm honestly not sure what value they will have to anyone else, and I'm hesitant to add testing code to drakma proper if I'm the only one likely to run it (and I'm fairly unlikely to run it after my itch is scratched). If you'd like that, then I'll put together a separate patch with a drakma-tests.asd to sort out the dependencies.

---

drakma-devel mailing list drakma-devel@common-lisp.net http://common-lisp.net/cgi-bin/mailman/listinfo/drakma-devel

Ok, I think the attached patch has just about everything:

* exports *remove-duplicate-cookies-p* * defaults *remove-duplicate-cookies-p* to T * adds documentation referencing the RFC (available at http://ryepup.unwashedmeme.com/blog/wp-content/uploads/2010/04/index.html)

Thanks, Ryan