zsoldosp / django-currentuser

Conveniently store reference to request user on thread/db level.
BSD 3-Clause "New" or "Revised" License
141 stars 40 forks source link

IntegrityError when running django tests #55

Open FinnGu opened 2 years ago

FinnGu commented 2 years ago

In my tests, I create an object that has an CurrentUserField. Up until recently everything worked, but now it fails with an SQLdb.IntegrityError after upgrading to python 3.9, django 3.2 and django-currentuser 0.5.3.

I was able to resolve this by explicitly overriding the user in the setUp() method of my test class as follows: _set_current_user(my_user). Is that the recommended approach? If so, we should document it somewhere.

Stacktrace:

ERROR: test_donation_bill (app.fairgeben.tests.test_donation.DonationInternalsTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/mysql/base.py", line 73, in execute
    return self.cursor.execute(query, args)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/cursors.py", line 206, in execute
    res = self._query(query)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/cursors.py", line 312, in _query
    db.query(q)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/connections.py", line 224, in query
    _mysql.connection.query(self, query)
MySQLdb._exceptions.IntegrityError: (1452, 'Cannot add or update a child row: a foreign key constraint fails (`test_edonapi`.`fairgeben_donationcomment`, CONSTRAINT `fairgeben_donationco_created_by_id_5e9223c0_fk_users_use` FOREIGN KEY (`created_by_id`) REFERENCES `users_user` (`id`))')

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/src/app/fairgeben/tests/test_donation.py", line 290, in test_donation_bill
    self.assertRaises(
  File "/usr/local/lib/python3.9/unittest/case.py", line 733, in assertRaises
    return context.handle('assertRaises', args, kwargs)
  File "/usr/local/lib/python3.9/unittest/case.py", line 201, in handle
    callable_obj(*args, **kwargs)
  File "/usr/src/app/fairgeben/models.py", line 631, in add_to_accounting
    self._log_invalid_status_change(
  File "/usr/src/app/fairgeben/models.py", line 553, in _log_invalid_status_change
    DonationComment.objects.create(
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/query.py", line 453, in create
    obj.save(force_insert=True, using=self.db)
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/base.py", line 726, in save
    self.save_base(using=using, force_insert=force_insert,
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/base.py", line 763, in save_base
    updated = self._save_table(
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/base.py", line 868, in _save_table
    results = self._do_insert(cls._base_manager, using, fields, returning_fields, raw)
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/base.py", line 906, in _do_insert
    return manager._insert(
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/query.py", line 1270, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
  File "/opt/venv/lib/python3.9/site-packages/django/db/models/sql/compiler.py", line 1416, in execute_sql
    cursor.execute(sql, params)
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 66, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 75, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/opt/venv/lib/python3.9/site-packages/django/db/utils.py", line 90, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/opt/venv/lib/python3.9/site-packages/django/db/backends/mysql/base.py", line 73, in execute
    return self.cursor.execute(query, args)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/cursors.py", line 206, in execute
    res = self._query(query)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/cursors.py", line 312, in _query
    db.query(q)
  File "/opt/venv/lib/python3.9/site-packages/MySQLdb/connections.py", line 224, in query
    _mysql.connection.query(self, query)
django.db.utils.IntegrityError: (1452, 'Cannot add or update a child row: a foreign key constraint fails (`test_edonapi`.`fairgeben_donationcomment`, CONSTRAINT `fairgeben_donationco_created_by_id_5e9223c0_fk_users_use` FOREIGN KEY (`created_by_id`) REFERENCES `users_user` (`id`))')
ivan-price-acted commented 2 years ago

Hi there

I believe i ran into a variant of this as well, the function get_current_authenticated_user() works fine but our tests fail with errors such as:

E               psycopg2.errors.ForeignKeyViolation: insert or update on table "core_country" violates foreign key constraint "core_country_created_by_id_df63df11_fk_core_user_id"
E               DETAIL:  Key (created_by_id)=(6) is not present in table "core_user".

it seems that during the setup of subsequent tests the 'current user' is still set, and the call to get_current_authenticated_user() in the model save() method is trying to reference a non-existing user.

Our fix is to clear the current user after the response = self.get_response(request) line here:

https://github.com/PaesslerAG/django-currentuser/blob/master/django_currentuser/middleware.py#L34

e.g. _set_current_user(None).

Is there a reason why this is not cleared in the code already ?

otherwise thanks for the module :)

-ivan

alfonsrv commented 2 years ago

@ivan-price-acted did you modify the middleware or how exactly did you do that?

Having the exact same issue when running tests. Somebody else has suggested a fixture here https://github.com/PaesslerAG/django-currentuser/issues/47

ivan-price-acted commented 2 years ago

@alfonsrv indeed we run a version of the middleware where just after the line:

https://github.com/PaesslerAG/django-currentuser/blob/master/django_currentuser/middleware.py#L34

, i.e. once we have the response, we set the user to None.

We've not observed any bad side effects, and it avoids having to use special fixtures for the tests.

To be mega explicit:

    def __call__(self, request):
        # request.user closure; asserts laziness;
        # memorization is implemented in
        # request.user (non-data descriptor)
        _do_set_current_user(lambda self: getattr(request, 'user', None))
        response = self.get_response(request)
        # our modification here to prevent tests from failing.
        _set_current_user(None)
        # end modif
        return response

I'd be interested to know what dangers may befall us, for now it works.

-i

alfonsrv commented 2 years ago

Yeah well it feels like it should be the default way to me too that the user is unset after requests. To be thread-safe and avoid the possibility of leaking information.

There already seems to be a PR regarding the topic, but it's stalled https://github.com/PaesslerAG/django-currentuser/pull/49 - tests...

ivan-price-acted commented 2 years ago

ah ok i hadn't found that PR.

looking at the test that fails for me it looks like the test is not good, it is asserting the 'current user' after the request is finished.

https://github.com/PaesslerAG/django-currentuser/blob/master/tests/testapp/tests.py#L46

the assert should be inside the handling for the view, and client.login() could probably be used to avoid the call to the login endpoint which maybe adds complexity for not much gain.