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

[Jeff Balogh]

#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.

[Ramiro Morales]

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

[Jeff Balogh]

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.

[Jacob]

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

[Aymeric Augustin]

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.

[Jacob]

Milestone 1.4 deleted

[btimby]

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.

[btimby]

Test Client now checks for middleware that DERIVES from SessionMiddleware.

[Russell Keith-Magee]

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...')

[Ramiro Morales]

Patch from btimby, plus tests

[Ramiro Morales]

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

[Ramiro Morales]

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()?

2. 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?

[Carl Meyer]

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?

[Claude Paroz]

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.

[Jannis Leidel]

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.

[Jannis Leidel]

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.

[Ramiro Morales]

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

[Ramiro Morales]

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.