Opened 8 years ago

Last modified 2 years ago

#8122 new Cleanup/optimization

Better way of testing for cookies

Reported by: Joost Cassee Owned by: nobody
Component: contrib.sessions Version: master
Severity: Normal Keywords:
Cc: joost@…, Domen Kožar Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


While trying to get the cookie test in the login form working again by default (see ticket #8061) I think I found a way to get rid of the set_test_cookie() / test_cookie_worked() dance.

The attached patch makes the session middleware set an empty cookie if the session does not yet have any variables set. This makes it possible to check whether the browser accepts cookies without poluting the session store. You can check for cookies using accepts_cookies() method. The old methods are marked deprecated but still work.

Attachments (3)

8122-r8161.diff (4.1 KB) - added by Joost Cassee 8 years ago.
8122-r8277.diff (7.8 KB) - added by Joost Cassee 8 years ago.
8122-rmaster.diff (7.9 KB) - added by Ed Henderson 19 months ago.
Updated the diff to the latest master

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by Joost Cassee

Attachment: 8122-r8161.diff added

comment:1 Changed 8 years ago by Joost Cassee

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

The new patch 8122-r8277.diff renames the session.accepts_cookies() method to the session.cookie_received property to match the semantics of the check.

The patch also includes tests to show the procedure works. I'm setting triage to 'design decision needed' because I feel the patch is in good shape to be judged by the Django devs. If accepted an update to the session docs needs to be included.

Changed 8 years ago by Joost Cassee

Attachment: 8122-r8277.diff added

comment:2 Changed 8 years ago by Joost Cassee

After applying patch 8122-r8277.diff, add an empty file in the sessions_regress directory. I couldn't get svn diff to include it.

comment:3 Changed 8 years ago by Chris Beaven

Why the change in behaviour to set an initial session cookie for an unmodified session (if it wasn't found)?

comment:4 Changed 8 years ago by Joost Cassee

The idea is that every session has a session cookie so that you avoid the need to call set_test_cookie() (which sets the cookie by added to the session). The session cookie is empty if the session is not yet modified to avoid a call to the session backend.

comment:5 Changed 8 years ago by Chris Beaven

Ah, I understand now - I was just a bit worried that it would be picking up that the key wasn't None and doing something with it. After a closer look at the session innards I see this isn't the case.

This ticket looks good - why don't you raise it on the django-dev group?

Of minor importance - the Django test client isn't really necessary here is it? (it's a lot slower than the testcase one) and wouldn't it be better to use the proper testcase assertions rather than just assert (but I'm not sure if it really matters)?

comment:6 Changed 8 years ago by Joost Cassee

I agree that the difference between the session key being None and being "" is a subtle one, yet it is a useful distinction here. I wrote tests precisely to check whether I did the tests right!

The tests were written before I noticed how other middleware was tested. I agree that changing it to just calling the middleware directly would be nice.

I will post a message about this ticket on django-dev, as you suggested. Thanks for looking at it!

comment:7 Changed 6 years ago by Domen Kožar

Cc: Domen Kožar added

comment:8 Changed 6 years ago by Luke Plant

Severity: Normal
Type: Cleanup/optimization

comment:9 Changed 5 years ago by Alex Gaynor

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

comment:10 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Patch no longer applies cleanly.

Changed 19 months ago by Ed Henderson

Attachment: 8122-rmaster.diff added

Updated the diff to the latest master

Note: See TracTickets for help on using tickets.
Back to Top