fix: infinite recursion bug on django-polymorphic >=4.9#1571
Merged
fsbraun merged 1 commit intodjango-cms:masterfrom Jan 17, 2026
Merged
fix: infinite recursion bug on django-polymorphic >=4.9#1571fsbraun merged 1 commit intodjango-cms:masterfrom
fsbraun merged 1 commit intodjango-cms:masterfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns File model manager configuration with newer django-polymorphic behavior to avoid infinite recursion when accessing related managers, and updates package constraints and schema to persist the new base manager setting. Sequence diagram for Folder deletion using File base_manager_namesequenceDiagram
actor Admin as Admin
participant FolderModel as Folder
participant DjangoORM as Django_ORM
participant FileRelatedManager as File_related_manager
participant FileObjectsManager as File_objects_manager
participant FileQuerySet as PolymorphicQuerySet_with_only_override
Admin->>DjangoORM: call delete on Folder instance
DjangoORM->>FolderModel: Folder.delete()
FolderModel->>FileRelatedManager: all_files.only(id).delete()
Note over FileRelatedManager,FileObjectsManager: After this PR base_manager_name = objects
FileRelatedManager->>FileObjectsManager: resolve base manager objects
FileObjectsManager->>FileQuerySet: only(id)
FileQuerySet-->>FileObjectsManager: QuerySet with id,_file_size,is_public
FileObjectsManager->>FileQuerySet: delete()
FileQuerySet-->>DjangoORM: cascade delete File rows without recursion
Class diagram for updated File model Meta optionsclassDiagram
class File{
+FileMeta Meta
}
class FileMeta{
+string app_label = filer
+string verbose_name = file
+string verbose_name_plural = files
+string base_manager_name = objects
}
File *-- FileMeta
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
f720d76 to
d909da1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1571 +/- ##
==========================================
+ Coverage 76.95% 76.98% +0.03%
==========================================
Files 77 78 +1
Lines 3666 3671 +5
Branches 498 498
==========================================
+ Hits 2821 2826 +5
Misses 675 675
Partials 170 170 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The django-polymorphic version constraint now allows 4.9.0 and 4.10.0–4.10.2, which you’ve identified as broken; consider explicitly excluding these versions (e.g. using
!=constraints) so that users can’t install them accidentally. - The
AlterModelOptionsmigration hardcodesverbose_nameandverbose_name_plural; if these are ever changed on the model, remember that this migration will still reflect the old values, so it may be safer to generate the migration from the current model state rather than editing by hand.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The django-polymorphic version constraint now allows 4.9.0 and 4.10.0–4.10.2, which you’ve identified as broken; consider explicitly excluding these versions (e.g. using `!=` constraints) so that users can’t install them accidentally.
- The `AlterModelOptions` migration hardcodes `verbose_name` and `verbose_name_plural`; if these are ever changed on the model, remember that this migration will still reflect the old values, so it may be safer to generate the migration from the current model state rather than editing by hand.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
4.9.0 and 4.10.0, 4.10.1, 4.10.2 were all yanked so this should be fine. |
Contributor
Author
Not valid. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
closes #1569
There are a few things going on here:
base_manager(used for loaddata and related fields) was set to the same as thedefault_managerby default. This was incongruous with Django's behavior so it was fixed in django-polymorphic/#816.PolymorphicQuerySetis used onFileobjects as the default manager. It overridesonly()to always add a few fields including_file_sizeandis_publicbecause these fields are accessed in the model's__init__method. I suspect this infinite recursion bug was encountered before, hence the override.Folderobject, internally Django does something like:folder.all_files.only("id").delete(). This triggered the bug because it was now using a plainPolymorphicManagerthat did not add the extra fields toonly().base_manager_name = "objects"to force related managers to use the overriddenonly()implementation.This PR, combined with the fixes in 4.9.1 and 4.10.3 should fix everything. It will also work on versions prior to 4.9.
You probably want to merge this PR soon because if anyone installs django-polymorphic 4.9+ with the existing release they will not be able to delete Folders that aren't empty. I am also a user of this great library so thank you!!
I also restricted the django-polymorphic dependency to < 5. As the maintainer I can attest that there will be breaking changes (but for great reasons!)
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Update filer File model manager configuration to align with newer django-polymorphic behavior and prevent recursion issues when deleting related objects.
Bug Fixes:
Build:
Chores: