Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#16605 closed Bug (duplicate)

Can't client.login() in tests if contrib.SessionMiddleware is not in MIDDLEWARE_CLASSES

Reported by: Jeff Balogh Owned by: Ramiro Morales
Component: Testing framework Version: 1.3
Severity: Release blocker Keywords:
Cc: django@…, charette.s@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

#7836 ([16386]) changed django.test.client.Client to require 'django.contrib.sessions.middleware.SessionMiddleware' in settings.MIDDLEWARE_CLASSES. This is extremely brittle; I have a subclass of SessionMiddleware in my MIDDLEWARE_CLASSES but of course the paths don't match, so now client.login() fails.

Attachments (3)

test-client-session-detection.patch (1.1 KB ) - added by btimby 12 years ago.
Test Client now checks for middleware that DERIVES from SessionMiddleware.
16605-with-tests.diff (4.4 KB ) - added by Ramiro Morales 12 years ago.
Patch from btimby, plus tests
16605-with-tests-2.diff (7.4 KB ) - added by Ramiro Morales 12 years ago.
Another interation of the patch, with addition of a helper function as suggested by Russell

Download all attachments as: .zip

Change History (19)

comment:1 by Ramiro Morales, 13 years ago

Triage Stage: UnreviewedDesign decision needed

Would sub-classing the test Client to accompany your custom SessionMiddleware subclass be of help in your situation?

in reply to:  1 comment:2 by Jeff Balogh, 13 years ago

Replying to ramiro:

Would sub-classing the test Client to accompany your custom SessionMiddleware subclass be of help in your situation?

I would have to copy over the full definition of login() to change that one line. So yes, it's possible, but it's not pretty.

comment:3 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

Yeah, the check introduced in [16386] is unreasonably brittle. There's probably a way of checking for session support beyond peeking at MIDDLEWARE_CLASSES.

comment:4 by Aymeric Augustin, 13 years ago

This pattern also happens in other parts of Django.

I remember that the admin has an hardcoded check for the auth middleware when DEBUG is True. I've ended up:

  • including both Django's middleware and my subclass in development.
  • including only my subclass in production.

It would be very nice to make these checks less stringent — at least check for a subclass.

comment:5 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:6 by btimby, 12 years ago

Hi, I suffered from this change as well when updating to Django 1.4. I am using a derived SessionMiddleware class that allows views to "opt-out" of updating the session expiry.

Checking for a subclass would resolve my woes as well. I will attach a patch.

by btimby, 12 years ago

Test Client now checks for middleware that DERIVES from SessionMiddleware.

comment:7 by Russell Keith-Magee, 12 years ago

Severity: NormalRelease blocker

Marking as a release blocker because it's a backwards incompatible change. Even if the fix for release purposes is documentation-only, this is something we should address before final.

The proposed patch looks like a reasonable approach, but it needs tests, and it feels like something that might be a useful general capability for other parts of the codebase (i.e., 'dot-path named class is a subclass of...')

comment:8 by Ramiro Morales, 12 years ago

Owner: changed from nobody to Ramiro Morales
Status: newassigned

by Ramiro Morales, 12 years ago

Attachment: 16605-with-tests.diff added

Patch from btimby, plus tests

by Ramiro Morales, 12 years ago

Attachment: 16605-with-tests-2.diff added

Another interation of the patch, with addition of a helper function as suggested by Russell

comment:9 by Ramiro Morales, 12 years ago

Has patch: set

16605-with-tests-2.diff contains:

  • The fix proposed by contributor btimby
  • Tests were added
  • A helper issubclass_by_name(symbol_name, klass) helper function in django/utils/module_loading as suggested by Russell that implements functionality like this:
>>> issubclass_by_name('django.utils.datastructures.SortedDict', dict) 
True
  • Tests for issubclass_by_name

Now, there are two issues I'd like to get some feedback:

  1. In the commit I introduced this change in behavior ([16386]) two Client methods were modified: _session() (that implements the session property) and login() to use the brittle strategy of testing with if 'django.contrib.sessions.middleware.SessionMiddleware' in settings.MIDDLEWARE_CLASSES. Now, because of this ticket we are undoing the change in login(). Do you think we should do the same in _session()?
  1. I'm still not totally sure the new proposed strategy of testing if the middleware in use is a subclass of django.contrib.sessions.middleware.SessionMiddleware is completely correct because it defeats the possibility of using duck-typing. To this effect I added a failing test case that exercises the code with a custom session middleware that doesn't. Do you think it is worth (i.e. it is valid to talk about request.session in the Django test Client when the session backend is a third party implementation)? or in such case all bets off and we simply should remove the test case?
Version 0, edited 12 years ago by Ramiro Morales (next)

comment:10 by Carl Meyer, 12 years ago

I think that a) importing a dotted-path-class and checking if its a subclass of some other class is an ugly pattern to introduce, and b) it's still too brittle with regard to duck-typing, as Ramiro notes. I think I'd almost rather just see r16386 reverted; not ideal, but having to have "contrib.sessions" in INSTALLED_APPS is arguably reasonable if you are using contrib.sessions, even if it does result in an unused database table created.

To back up a step, what's the actual need to have this check at all? If someone is using the test client and tries to access the session attribute or call .login() and they aren't "using" contrib.sessions, what's the actual impact? Seems like that might be a "well, if it hurts then don't do it" scenario. And if we're having such trouble defining what it means to be "using" contrib.sessions (might not have it in INSTALLED_APPS, might not be using its middleware...), maybe we should stop trying to check for something that is so poorly defined?

comment:11 by Claude Paroz, 12 years ago

I think basically that we miss a way to know if sessions are activated or not. What about using SESSION_ENGINE for this? That would change the logic to an opt-out choice: "Sessions are enabled by default, if you don't need session support, set SESSION_ENGINE to an empty string and remove 'django.contrib.sessions' from your INSTALLED_APPS setting". I guess 98% of projects do use sessions anyway.

comment:12 by Jannis Leidel, 12 years ago

We don't have any other setting that can be set to an empty string, so this is definitely something I wouldn't prefer for this.

I'd be in favor of reverting r16386, too.

comment:13 by Jannis Leidel, 12 years ago

To clarify, we do have settings that can have a empty string as a valid value, just don't use that state to disable a feature.

comment:14 by Ramiro Morales, 12 years ago

In [17739]:

Reverted r16386 because it replaced a brittle method with another not less
arbitrary method when the test client checks for the presence of the bundled
session backends+session middleware combination.

We will revisit the issue soon, probably to make these checks even less strict.

Refs #7836, #16605

comment:15 by Ramiro Morales, 12 years ago

Resolution: duplicate
Status: assignedclosed

Closing as duplicate of #7836. We will push for a more general solution there, be it adding or be it removing this kind of checks.

comment:16 by Simon Charette, 12 years ago

Cc: charette.s@… added
Note: See TracTickets for help on using tickets.
Back to Top