Code

Opened 3 years ago

Closed 3 years ago

#15929 closed Bug (fixed)

test.client.RequestFactory keeps state

Reported by: m.vantellingen@… Owned by: nobody
Component: Testing framework Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

See the attached testcase. When a unittest calls self.client.get() once all futher calls to RequestFactory.get() result in different request objects (the user and session are available).

Attachments (5)

tests.py (735 bytes) - added by m.vantellingen@… 3 years ago.
Test for the error
tests-2.py (1.0 KB) - added by aaugustin 3 years ago.
django-patch-15929.diff (760 bytes) - added by mvantellingen 3 years ago.
15929-fix.diff (840 bytes) - added by lukeplant 3 years ago.
Fix by making AuthenticationMiddleware not do naughty monkey patching
15929.diff (2.3 KB) - added by ramiro 3 years ago.
Fix bu Luke Plant integrated with tests by m.vantellingen AT auto-interactive.nl and aaugustin

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by m.vantellingen@…

Test for the error

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I confirm the issue. I attach a slightly modified TestCase that works out of the box and makes the problem more obvious.

Removing either self.client.get('/') or request.session = {} from common_test_that_should_always_pass make the problem disappear. It looks like there is some shared state between the TestClient and the RequestFactory, where the docs only mention a shared API.

The state leakage between the tests is probably related, and it's pretty bad, since the same test can pass or fail depending on its position in the TestCase.

Changed 3 years ago by aaugustin

comment:2 Changed 3 years ago by aaugustin

Oops, §2 above should read:

Removing either self.client.get('/') from test_request_after_client or request.session = {} from common_test_that_should_always_pass makes the problem disappear.

comment:3 Changed 3 years ago by mvantellingen

  • Has patch set

The issue seems to be in the AuthenticationMiddleware which patches the WSGIRequest class to add the LazyUser() object. The simplest solution seems to be to remove the user attribute on the created request in the RequestFactory. See attached patch

Changed 3 years ago by mvantellingen

comment:4 Changed 3 years ago by aaugustin

  • Needs tests set

I haven't looked into the details, but this patch looks like it's fixing the symptoms and not addressing the root cause of the problem.

For instance, what if someone has a custom middleware that adds another variable?

comment:5 Changed 3 years ago by mvantellingen

Well the root cause is obviously that the AuthenticationMiddleware modifies the WSGIRequest class.
The proper solution is to actually not do this, but there are probably reasons why this is done :-)

I'm not sure if django should support custom middleware which modifies django internals.


Changed 3 years ago by lukeplant

Fix by making AuthenticationMiddleware not do naughty monkey patching

comment:6 Changed 3 years ago by lukeplant

The authentication does indeed patch the request class for a reason, but there is way to avoid it, which is in the patch I just attached. I haven't actually checked that my patch fixes the issue, but I'm pretty sure it does, and all the other tests pass.

I'm happy with this approach, but not with just fixing the symptoms as in the alternative approach. If someone integrates the regression tests for this into a full patch, I'll mark it as RFC.

Changed 3 years ago by ramiro

Fix bu Luke Plant integrated with tests by m.vantellingen AT auto-interactive.nl and aaugustin

comment:7 Changed 3 years ago by ramiro

  • Needs tests unset

comment:8 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

In [16297]:

Fixed #15929 - test.client.RequestFactory keeps state/AuthMiddleware does monkey patching

Thanks to m.vantellingen for the report and tests, and to aaugustin for
work on the tests.

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.