-
-
Notifications
You must be signed in to change notification settings - Fork 300
Support Q expressions that contain subquery expressions #572
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
Conversation
|
Thanks for opening a PR. Could you add the following:
|
842e480 to
acf288a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #572 +/- ##
==========================================
+ Coverage 75.40% 75.50% +0.09%
==========================================
Files 21 21
Lines 1342 1343 +1
Branches 211 212 +1
==========================================
+ Hits 1012 1014 +2
Misses 257 257
+ Partials 73 72 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
acf288a to
79b2b9a
Compare
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 fixes a bug in the polymorphic query translation logic where Q expressions containing subquery expressions (like Exists) were not handled correctly. When Exists subqueries are combined using the | (OR) operator, Django creates a Q object with Exists instances as children, but the old code incorrectly assumed all non-tuple/list children of Q objects were themselves Q objects and attempted to recursively process them.
Key changes:
- Modified Q object tree traversal to explicitly check if children are Q objects before recursing
- Added comprehensive test coverage for the
Existssubquery scenario with multiple test cases - Added
related_nameto model field to support the new test queries
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/polymorphic/query_translate.py | Fixed Q object traversal to only recurse on Q children, not all non-tuple children |
| src/polymorphic/tests/test_orm.py | Added comprehensive test for Exists subqueries combined with OR operator |
| src/polymorphic/tests/models.py | Added related_name="inline_bs" to InlineModelB.plain_a field for test support |
| src/polymorphic/tests/migrations/0001_initial.py | Updated migration to reflect model changes |
| docs/changelog.rst | Added changelog entry (currently commented out) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: CelestialGuru <45701317+CelestialGuru@users.noreply.github.com> Co-authored-by: Brian Kohan <bckohan@gmail.com>
5914ae3 to
6372ce1
Compare
bckohan
left a comment
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.
This fixes a real bug - I have added tests. Thank you!
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes bug when using
Existssubqueries which are or-ed together:This is done usually for performance reasons, but it is a valid queryset.
Existinstances ored together like this creates aQinstance, but the children are not themselvesQinstances. They areExistsinstances.Fixed #323