Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mrmachine opened this issue Mar 18, 2020 · 8 comments

Comments

@mrmachine
Copy link

# 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
Copy link
Author

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?

@Suor
Copy link
Owner

Suor commented Mar 19, 2020

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.

@lampwins
Copy link
Contributor

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')

@mrmachine
Copy link
Author

Oops. Didn't mean to close this.

@mrmachine mrmachine reopened this May 15, 2020
@Suor
Copy link
Owner

Suor commented Jun 21, 2020

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

@voron3x
Copy link

voron3x commented Aug 16, 2020

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 #371

@Suor
Copy link
Owner

Suor commented Aug 30, 2020

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

Suor added a commit that referenced this issue Oct 25, 2020
ducva pushed a commit to ducva/django-cacheops that referenced this issue Jul 30, 2021
@ducva
Copy link

ducva commented Jul 30, 2021

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.

g-kartik added a commit to g-kartik/cvat that referenced this issue Jan 2, 2022
Task and Job models have many to one relationship with User model on assignee and reviewer fields. And on deleting the parent model, these fields are set to NULL.
However due to an open issue in django-cacheops package, the related querysets are not invalidated upon deletion of parent model. Refer - Suor/django-cacheops#348.
So we have to manually invalidate the cached querysets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants