-
-
Notifications
You must be signed in to change notification settings - Fork 300
Fix proxy model support. #676
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #676 +/- ##
==========================================
+ Coverage 75.39% 75.57% +0.18%
==========================================
Files 21 21
Lines 1345 1355 +10
Branches 212 216 +4
==========================================
+ Hits 1014 1024 +10
Misses 259 259
Partials 72 72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new polymorphic__proxy meta option to control whether proxy models behave as traditional Django proxies (aliases) or as polymorphic proxies (distinct types). This addresses issues #376 and #390 by allowing developers to opt-in to traditional Django proxy behavior when needed.
Key Changes
- Introduced
polymorphic__proxymeta attribute that can be set toTrue(traditional Django proxy/alias) orFalse(polymorphic proxy with distinct type) - Modified the manager's queryset logic to respect the new attribute when applying
instance_offilters - Added comprehensive test coverage with multiple proxy model variants to validate the new behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/polymorphic/base.py | Added metaclass logic to extract and store polymorphic__proxy from model Meta, with default based on proxy setting |
| src/polymorphic/managers.py | Changed queryset logic from checking _meta.proxy to checking _meta.polymorphic__proxy to control instance filtering |
| src/polymorphic/tests/models.py | Added 6 new test model classes demonstrating different proxy configurations and inheritance hierarchies |
| src/polymorphic/tests/test_orm.py | Added 6 new test cases to validate polymorphic proxy behavior, traditional Django proxy behavior, and type conversion scenarios |
| src/polymorphic/tests/migrations/0001_initial.py | Generated migration file for the new test models |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27c6473 to
a83683d
Compare
a83683d to
c626431
Compare
|
On further consideration I'm bumping this PR to version 5 because I'm unsatisfied with any backwards compatible interface. I do not believe by default concrete models should return proxy instances and that this behavior should be toggleable by using polymorphic_proxy flag. |
Fixes #376 #390
I need to rebase and didn't have permissions to push to the original branch for #544, so this replaces that PR.
When polymorphic support was added to django-polymorphic circa 2013-2014 the design decision was to treat proxy models as concrete models wrt to polymorphism and to restrict the managers to those instances. This works because for_concrete_model=False is passed when the the ContentTypes are tagged onto the models - so if a row is created from the proxy model the proxy model's ContentType will be used to tag it. This is how you get situations described by #376:
I think this is pretty confusing default behavior because the main point of a proxy model is to provide a different python interface to an existing db row and this by default both treats the proxies as polymorphic extensions and restricts the querysets to rows created from the proxy instance. There is certainly a case for it, and Django allows the ContentType system to opt into proxy distinctions (thought not by default) - I just think it would have made more sense for the default behavior to be what people naturally should expect.
Where to go from here:
The new interface - which is now documented - will be:
The other change that I'm going to add to @pgammans PR is for proxy models to behave like proxies in querysets as well. For example, from above: