django-cms / django-filer

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

Fix: Flie.objects.only("id") query when deleting a user. #1308

Closed wiltso closed 1 year ago

wiltso commented 1 year ago

Description

When deleting a User object, Django's django.db.models.deletion.Collector will make a query like this: File.objects.filter(owner=user).only("id"). This then leads to a recursion error due to that the filler.models.File object requires that the following fields are present:

Related resources

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #1308 (320c7ff) into master (34803bc) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1308      +/-   ##
==========================================
+ Coverage   72.39%   72.45%   +0.05%     
==========================================
  Files          72       72              
  Lines        3268     3275       +7     
  Branches      532      532              
==========================================
+ Hits         2366     2373       +7     
  Misses        735      735              
  Partials      167      167              
Impacted Files Coverage Δ
filer/models/filemodels.py 86.95% <100.00%> (+0.40%) :arrow_up:
vinitkumar commented 1 year ago

@wiltso Do you remember if the error you got is almost like this?

IntegrityError at /admin/customroles/siteuser/
update or delete on table "auth_user" violates foreign key constraint "filer_file_owner_id_b9e32671_fk_auth_user_id" on table "filer_file"
DETAIL: Key (id)=(89) is still referenced from table "filer_file".
wiltso commented 1 year ago

I was actually getting an recursion error. You can replicate it by doing File.objects.filter(owner=u).only("id").

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.

fsbraun commented 1 year ago

@wiltso Sorry, that this got "lost". I would like to take it up. I'd really appreciate, if we could add a test for it:

Do you think you could add such an test?

fsbraun commented 1 year ago

@wiltso I've created a follow-up PR (#1357) that includes the cherry-pick of this PR and a simple test. I really would like to get this fix merged now.

fsbraun commented 1 year ago

Implemented in #1357