Unset middleware request

[lucaswiman]

@treyhunner This pull request adds a process_response method to HistoryRequestMiddleware to unset the threadlocal request attribute when a response is processed. See also jedie/django-tools#7, where I did something similar. This behavior means there are no side-effects on global state which are not cleared by the django TestCase class.

Testing

I added tests for the desired behavior, including a regression test (test_rolled_back_user_does_not_lead_to_foreign_key_error) simulating the following scenario:

• A test executes, creates a user U and simulates that user saving a model instance in an admin form. This sets the HistoricalRecords.thread.request variable to a request instance referencing the user.
• The Django TestCase rolling back the creation of the user so that U doesn't exist in the database anymore. (Or, the user entry is deleted.)
• A new test executes, running only backend code. The history_user foreign key is set to a User that does not exist in the database, failing with the following error:

Traceback (most recent call last):
  File "/Users/lucaswiman/opensource/django-simple-history/simple_history/tests/tests/test_admin.py", line 231, in test_rolled_back_user_does_not_lead_to_foreign_key_error
    historical_book.history_user,
  File "/Users/lucaswiman/opensource/django-simple-history/.tox/py27-django19/lib/python2.7/site-packages/django/db/models/fields/related_descriptors.py", line 169, in __get__
    rel_obj = qs.get()
  File "/Users/lucaswiman/opensource/django-simple-history/.tox/py27-django19/lib/python2.7/site-packages/django/db/models/query.py", line 387, in get
    self.model._meta.object_name
DoesNotExist: CustomUser matching query does not exist.

[macro1]

Nice. I've been a little worried about the middleware, cleanup should help a little in preventing nasty surprises.

In your added test, did you verify the middleware is being called? I have a feeling there should be a constraint failure in what you're trying to demonstrate.

Another approach would be to set the request attribute on the thread local explicitly, instead of trying to mock the settings. You would then have the added advantage of giving it a ridiculous id that would never exist normally, like -1.

[lucaswiman]

Thanks! Do you know what's happening with the travis build? The failures superficially appear unrelated to my changes.

In your added test, did you verify the middleware is being called? I have a feeling there should be a constraint failure in what you're trying to demonstrate.

Yes, and I did see the constraint error, as hoped. The middleware is definitely being called, since I wrote the tests first in the git history. Running git checkout 5806e9b45067d3c5f3a8cc7d66008e7f14740212; tox -e py27-django19, you see the following failures:

...............F.....E......................................................................................
======================================================================
ERROR: test_rolled_back_user_does_not_lead_to_foreign_key_error (simple_history.tests.tests.test_admin.AdminSiteTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/lucaswiman/opensource/django-simple-history/simple_history/tests/tests/test_admin.py", line 231, in test_rolled_back_user_does_not_lead_to_foreign_key_error
    historical_book.history_user,
  File "/Users/lucaswiman/opensource/django-simple-history/.tox/py27-django19/lib/python2.7/site-packages/django/db/models/fields/related_descriptors.py", line 169, in __get__
    rel_obj = qs.get()
  File "/Users/lucaswiman/opensource/django-simple-history/.tox/py27-django19/lib/python2.7/site-packages/django/db/models/query.py", line 387, in get
    self.model._meta.object_name
DoesNotExist: CustomUser matching query does not exist.

======================================================================
FAIL: test_middleware_unsets_request (simple_history.tests.tests.test_admin.AdminSiteTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/lucaswiman/opensource/django-simple-history/simple_history/tests/tests/test_admin.py", line 213, in test_middleware_unsets_request
    self.assertFalse(hasattr(HistoricalRecords.thread, 'request'))
AssertionError: True is not false

----------------------------------------------------------------------
Ran 108 tests in 3.745s

FAILED (failures=1, errors=1)

Since the tests pass on 4ecf837, and that commit only modifies the middleware, it must be getting called.

Another approach would be to set the request attribute on the thread local explicitly, instead of trying to mock the settings.

It seems like that would lose the benefit of testing the middleware's relationship to Django's request-response cycle.

You would then have the added advantage of giving it a ridiculous id that would never exist normally, like -1.

I'm not sure I see how that would work. Note that this doesn't test that saving a history model will work even if the user on the request object is invalid (it won't), but rather that the request is cleared at the end of the request-response cycle, so the data never has a chance to be invalid.

[macro1]

Basically, when you delete things in Django it goes out to all the related models to either cascade the delete or perform some 'on delete' action.

The missing tables are because those models were created inside a test case. I'm not sure what the right solution would be to allow deleting users without incident.

That was part of why I suggested attaching an unsaved user with an abnormal PK to the request instance... You wouldn't need to delete the user, and the strange errors would go away.

[lucaswiman]

I don't see a good way to set the .user attribute directly using the test client. However, I was able to prevent the Django cascade deletion accumulator from triggering by rolling back an atomic block where the user was created.

That didn't completely fix the issue, since the travis build is now failing on the test_deleteting_user test which is unrelated to this pull request. This seems like a pre-existing issue with the test suite.

[lucaswiman]

One option would be to use something like this stackoverflow post to dynamically add the app containing the model and sync it to the database.

Getting it to work in both 1.6/south and 1.7+ might be a bit tricky. If getting a working solution for 1.6 proves difficult, what are your thoughts about dropping support for Django 1.6? Note that 1.6 (and 1.7) have hit end-of-life, and aren't even receiving security updates anymore.

[macro1]

Cool. I'll look at it a bit and see if I can shake a solution out. Maybe we
can roll back the model registry or something in the other tests as well.
On Mar 17, 2016 3:19 PM, "Lucas Wiman" notifications@github.com wrote:

One option would be to use something like this stackoverflow post
http://stackoverflow.com/a/2672444 to dynamically add the app
containing the model and sync it to the database.

Getting it to work in both 1.6/south and 1.7+ might be a bit tricky. If
getting a working solution for 1.6 proves difficult, what are your thoughts
about dropping support for Django 1.6? Note that 1.6 (and 1.7) have hit
end-of-life, and aren't even receiving security updates anymore.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#213 (comment)

[macro1]

@lucaswiman: Don't forget to add yourself to the AUTHORS file before we get this merged in!

[lucaswiman]

Don't forget to add yourself to the AUTHORS file before we get this merged in!

Done, and I also updated the version and changelog. I think this is good to go on my end.

[lucaswiman]

Build failed with the same test_deleteting_user error.

[macro1]

Weird. And I used to be able to restart specific portions of the matrix build, but I don't seem to see that now. I'll work on resolving the test issues, thanks for getting this pr together!