django-cacheops icon indicating copy to clipboard operation
django-cacheops copied to clipboard

`get` operation raises `DoesNotExist` raised when related FK with `on_delete=SET_NULL` is deleted

Open mrmachine opened this issue 5 years ago • 8 comments

# models
class User(Model):
    active_device = ForeignKey(Device, on_delete=SET_NULL
    ...

# shell
In [3]: u = User.objects.cache().get(pk=4473)

In [4]: u.active_device
Out[4]: <Device: ...>

In [5]: u.active_device.delete()

In [6]: u = User.objects.cache().get(pk=4473)

In [7]: u.active_device
---------------------------------------------------------------------------
DoesNotExist                              Traceback (most recent call last)
<ipython-input-7-a017d853d610> in <module>()
----> 1 u.active_device

.../venv/lib/python2.7/site-packages/django/db/models/fields/related.py in __get__(self, instance, instance_type)
    318                     qs = qs.filter(extra_filter, **params)
    319                 # Assuming the database enforces foreign keys, this won't fail.
--> 320                 rel_obj = qs.get()
    321                 if not self.field.rel.multiple:
    322                     setattr(rel_obj, self.field.related.get_cache_name(), instance)

.../src/django-cacheops/cacheops/query.py in get(self, *args, **kwargs)
    344             qs = self
    345 
--> 346         return qs._no_monkey.get(qs, *args, **kwargs)
    347 
    348     if django.VERSION >= (1, 6):

.../venv/lib/python2.7/site-packages/django/db/models/query.py in get(self, *args, **kwargs)
    308             raise self.model.DoesNotExist(
    309                 "%s matching query does not exist." %
--> 310                 self.model._meta.object_name)
    311         raise self.model.MultipleObjectsReturned(
    312             "get() returned more than one %s -- it returned %s!" %

DoesNotExist: Device matching query does not exist.

I guess cacheops could do something like:

  • add a pre_delete signal handler that checks if there are any FKs in other models pointing to the deleted model
  • add a list of those objects to thread locals
  • in post_delete iterate the objects from thread locals and call invalidate_obj() on each

This could be quite slow?

mrmachine avatar Mar 18 '20 10:03 mrmachine

Here's an implementation:

def invalidate_on_delete_set_null_relations_pre_delete(sender, instance, **kwargs):
    """
    Discover related objects that have an FK with `on_delete=SET_NULL` that points to
    an object being deleted. Add a list of these related objects to the instance for
    later invalidation in a `post_delete` signal handler.

    We need to execute the query and save a list of objects here, because the FK fields
    will already be set null in `post_delete`.
    """
    objs = []
    for ro in type(instance)._meta.get_all_related_objects():
        if ro.field.rel.on_delete == models.SET_NULL:
            objs.extend(ro.model.objects.filter(**{ro.field.name: instance.pk}))
    instance._invalidate_on_delete_set_null_related_objs = objs


pre_delete.connect(invalidate_on_delete_set_null_relations_pre_delete)


def invalidate_on_delete_set_null_relations_post_delete(sender, instance, **kwargs):
    """
    Invalidate related objects that have an FK with `on_delete=SET_NULL` that points to
    an object being deleted, stored on the instance by a `pre_delete` signal handler.
    """
    from cacheops import invalidate_obj
    for obj in instance._invalidate_on_delete_set_null_related_objs:
        invalidate_obj(obj)


post_delete.connect(invalidate_on_delete_set_null_relations_post_delete)

Is there a better way to bulk invalidate than fetching all related instances into a big list and calling invalidate_obj over each?

mrmachine avatar Mar 18 '20 12:03 mrmachine

I think cacheops may simply invalidate user here, it will be refetched from db with field set null. Can you create a test for it? There is an instruction at the end of README.

Suor avatar Mar 19 '20 13:03 Suor

I ran into this same issue but in a slightly different context. I was able to solve it by adding the FK relation to a prefetch_related() clause which causes the invalidation logic to properly link that relation and invalidate it.

So while my exact case if different than the one referenced here, I assume this would still work:

User.objects.cache().get(pk=4473).prefetch_related('active_device')

lampwins avatar May 15 '20 06:05 lampwins

Oops. Didn't mean to close this.

mrmachine avatar May 15 '20 06:05 mrmachine

Will anyone be willing to write a test for this? The instruction is at the end of the README.

Suor avatar Jun 21 '20 06:06 Suor

I have the same problem with this case

    def test_delete_1(self):
        category = Category.objects.create()
        Post.objects.create(category=category)
        Post.objects.create(category=category)
        objs = Post.objects.all().only("pk", "title").select_for_update()
        # ...
        # Some logic here for create, update and then delete models
        # ...
        objs.filter(pk__in=Post.objects.all().values("pk")).delete()

Trace

Traceback (most recent call last):
  File "/Users/voron3x/ExternalProject/django-cacheops/tests/tests.py", line 976, in test_delete_1
    objs.filter(pk__in=Post.objects.all().values("pk")).delete()
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/db/models/query.py", line 747, in delete
    deleted, _rows_count = collector.delete()
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/db/models/deletion.py", line 435, in delete
    signals.post_delete.send(
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 177, in send
    return [
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 178, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/Users/voron3x/ExternalProject/django-cacheops/cacheops/query.py", line 493, in _post_delete
    invalidate_obj(instance, using=using)
  File "/Users/voron3x/ExternalProject/django-cacheops/cacheops/invalidation.py", line 36, in invalidate_obj
    invalidate_dict(model, get_obj_dict(model, obj), using=using)
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/funcy/decorators.py", line 39, in wrapper
    return deco(call, *dargs, **dkwargs)
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/funcy/flow.py", line 194, in post_processing
    return func(call())
  File "/Users/voron3x/ExternalProject/django-cacheops/cacheops/invalidation.py", line 99, in get_obj_dict
    value = getattr(obj, field.attname)
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/db/models/query_utils.py", line 149, in __get__
    instance.refresh_from_db(fields=[field_name])
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/db/models/base.py", line 632, in refresh_from_db
    db_instance = db_instance_qs.get()
  File "/Users/voron3x/ExternalProject/django-cacheops/cacheops/query.py", line 352, in get
    return qs._no_monkey.get(qs, *args, **kwargs)
  File "/Users/voron3x/.virtualenvs/django-cacheops/lib/python3.8/site-packages/django/db/models/query.py", line 429, in get
    raise self.model.DoesNotExist(
tests.models.Post.DoesNotExist: Post matching query does not exist.

PR https://github.com/Suor/django-cacheops/pull/371

voron3x avatar Aug 16 '20 19:08 voron3x

As far as I see @voron3x issue is different from the original one. They doesn't seem to be connected.

Suor avatar Aug 30 '20 11:08 Suor

I got the same problem, created PR #406 to reproduce the bug

ERROR: test_case (tests.tests.TestIssue348)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 173, in __get__
    rel_obj = self.field.get_cached_value(instance)
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/fields/mixins.py", line 15, in get_cached_value
    return instance._state.fields_cache[cache_name]
KeyError: 'design'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/michael/0_works/poc/django-cacheops/tests/tests.py", line 1059, in test_case
    self.assertIsNone(c.design)
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 187, in __get__
    rel_obj = self.get_object(instance)
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 307, in get_object
    return super().get_object(instance)
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/fields/related_descriptors.py", line 154, in get_object
    return qs.get(self.field.get_reverse_related_filter(instance))
  File "/Users/michael/0_works/poc/django-cacheops/cacheops/query.py", line 351, in get
    return qs._no_monkey.get(qs, *args, **kwargs)
  File "/Users/michael/.pyenv/versions/cacheops/lib/python3.8/site-packages/django/db/models/query.py", line 435, in get
    raise self.model.DoesNotExist(
tests.models.ChatBoxDesign.DoesNotExist: ChatBoxDesign matching query does not exist.

ducva avatar Jul 30 '21 09:07 ducva