Opened 7 years ago

Last modified 14 months ago

#8122 new Cleanup/optimization

Better way of testing for cookies

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

Description

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 jcassee 7 years ago.
8122-r8277.diff (7.8 KB) - added by jcassee 7 years ago.
8122-rmaster.diff (7.9 KB) - added by kutenai 4 months ago.
Updated the diff to the latest master

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by jcassee

comment:1 Changed 7 years ago by jcassee

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 7 years ago by jcassee

comment:2 Changed 7 years ago by jcassee

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

comment:3 Changed 7 years ago by SmileyChris

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

comment:4 Changed 7 years ago by jcassee

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 7 years ago by SmileyChris

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 7 years ago by jcassee

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 4 years ago by iElectric

  • Cc iElectric added

comment:8 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:9 Changed 4 years ago by Alex

  • Easy pickings unset
  • Triage Stage changed from Design decision needed to Accepted
  • UI/UX unset

comment:10 Changed 14 months ago by timo

  • Patch needs improvement set

Patch no longer applies cleanly.

Changed 4 months ago by kutenai

Updated the diff to the latest master

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