Code

Opened 3 years ago

Closed 2 years ago

Last modified 23 months ago

#16605 closed Bug (duplicate)

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

Reported by: jbalogh Owned by: ramiro
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 2 years ago.
Test Client now checks for middleware that DERIVES from SessionMiddleware.
16605-with-tests.diff (4.4 KB) - added by ramiro 2 years ago.
Patch from btimby, plus tests
16605-with-tests-2.diff (7.4 KB) - added by ramiro 2 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 follow-up: Changed 3 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:2 in reply to: ↑ 1 Changed 3 years ago by jbalogh

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 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to 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 Changed 3 years ago by aaugustin

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 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:6 Changed 2 years ago by 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.

Changed 2 years ago by btimby

Test Client now checks for middleware that DERIVES from SessionMiddleware.

comment:7 Changed 2 years ago by russellm

  • Severity changed from Normal to 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 Changed 2 years ago by ramiro

  • Owner changed from nobody to ramiro
  • Status changed from new to assigned

Changed 2 years ago by ramiro

Patch from btimby, plus tests

Changed 2 years ago by ramiro

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

comment:9 Changed 2 years ago by ramiro

  • 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 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?
Last edited 2 years ago by ramiro (previous) (diff)

comment:10 Changed 2 years ago by carljm

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 Changed 2 years ago by claudep

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 Changed 2 years ago by jezdez

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 Changed 2 years ago by jezdez

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 Changed 2 years ago by ramiro

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 Changed 2 years ago by ramiro

  • Resolution set to duplicate
  • Status changed from assigned to 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 Changed 23 months ago by charettes

  • Cc charette.s@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.