#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)
Change History (19)
follow-up: 2 comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:2 by , 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 , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
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 , 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:6 by , 13 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 , 13 years ago
Attachment: | test-client-session-detection.patch added |
---|
Test Client now checks for middleware that DERIVES from SessionMiddleware.
comment:7 by , 13 years ago
Severity: | Normal → Release 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 , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
by , 13 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 , 13 years ago
Has patch: | set |
---|
16605-with-tests-2.diff
contains:
- The fix proposed by contributor btimby
- Tests were added
- A
issubclass_by_name(symbol_name, klass)
helper function indjango.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:
- In the commit I introduced this change in behavior ([16386]) two Client methods were modified:
_session()
(that implements thesession
property) andlogin()
to use the brittle strategy of testing withif 'django.contrib.sessions.middleware.SessionMiddleware' in settings.MIDDLEWARE_CLASSES
. Now, because of this ticket we are undoing the change inlogin()
. Do you think we should do the same in_session()
?
- 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 aboutrequest.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?
comment:10 by , 13 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 , 13 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 , 13 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 , 13 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:15 by , 13 years ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
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 , 12 years ago
Cc: | added |
---|
Would sub-classing the test Client to accompany your custom SessionMiddleware subclass be of help in your situation?