django-cms / django-filer

File and Image Management Application for django
https://django-filer.readthedocs.io/
Other
1.76k stars 577 forks source link

[bug] Unable to delete any users that own assets #1290

Closed petrklus closed 1 year ago

petrklus commented 2 years ago

There seems to be an infinite loop that gets induced by trying to delete the user that is set as an owner of assets.

Steps to reproduce:

  1. Upload assets with user X
  2. Try to delete the user X

Instead of user deletion, the admin experiences page timeout and in the background, server reaches maximum recursion depth and crashes/restarts (depending on the setup).

The user is not deleted.

This has been reproduced on live installs, as well as fresh Django test benches. Local development server completely crashes (stack overflow).

Error information: https://sentry.io/share/issue/69ee18cd94f9459f923f83d02cb0d634/

Django Filer 2.0.2 and 2.2 Django 3.2.13.0 Django CMS 3.9.0.5

Apart from fixing the bug itself, I wonder if it may be worth having a unit test that covers this scenario?

This seems to be a new behaviour when using Django 3.2. A setup with Django 2.2 (but otherwise identical) does not seem to suffer from this issue.

petrklus commented 2 years ago

I think I have narrowed it down to a change in Django, which is now actively deferring fields that are not needed in certain operations, for example, deletions (https://code.djangoproject.com/ticket/31435).

In a nutshell, there should never be any touching of potentially deferred fields in model __init__, as that causes another fetch, another init method… ad infinitum. This is an issue in the underlying File model (https://github.com/django-cms/django-filer/blob/master/filer/models/filemodels.py#L162).

My “hotfix” seems to be to detect whether the required fields are indeed in place and if not, to not attempt to full initialisation. Something along the lines of:

@classmethod
def from_db(cls, db, field_names, values):
    instance = super().from_db(db, field_names, values)
    full_init_fields = ['sha1', 'is_public', 'file']
    if all( [ (req_key in field_names) for req_key in full_init_fields]):
        print("FULL INIT")
        instance._old_is_public = instance.is_public
        instance.file_data_changed(post_init=True)
    else:
        print("PARTIAL INIT")

    return instance

Which replaces the __init__ function.

Any idea whether the above is a good long-term solution or can it cause problems down the line?

EDIT:

It seems that the from_db is not working in subclasses, moving the logic into __init__ seems to work better:

def __init__(self, *args, **kwargs):
    super().__init__(*args, **kwargs)

    # only perform the full init when the required fields
    # are loaded (not deferred)
    full_init_fields = ['sha1', 'is_public', 'file']
    if all( [ (req_key in self.__dict__) for req_key in full_init_fields]):
        print("FULL INIT")
        self._old_is_public = self.is_public
        self.file_data_changed(post_init=True)
    else:
        print("PARTIAL INIT")
stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

petrklus commented 2 years ago

I believe this affects Django 4.x as well.

wiltso commented 2 years ago

Yea I'm having the same issue, the underlying problem is when you do a .only() query. You will get the same infinite loop if you just do File.objects.filter(owner=u).only("id") and it work fine if you just do File.objects.filter(owner=u). When deleting the user model the Collector.collect() will do the .only query.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

petrklus commented 1 year ago

@wiltso any luck getting your fix accepted/reviewed?

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This will now be closed due to inactivity, but feel free to reopen it.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

petrklus commented 1 year ago

@wiltso any luck getting your fix accepted/reviewed? (bump)

fsbraun commented 1 year ago

@petrklus @wiltso Can we close this issue after the fix has been merged and released?

stale[bot] commented 1 year ago

This will now be closed due to inactivity, but feel free to reopen it.