Skip to content

Fixed #33319 -- Fixed crash when combining with the | operator querysets with aliases that conflict. - #15128

Merged
felixxm merged 3 commits into
django:mainfrom
omerfarukabaci:ticket_33319
Dec 8, 2021
Merged

felixxm merged 3 commits into
django:mainfrom
omerfarukabaci:ticket_33319

Conversation

@omerfarukabaci

Copy link
Copy Markdown
Contributor
  • Possible AssertionError during QuerySet's OR operation is fixed.
  • Regression tests are added.
  • AssertionError is documented in the code via comments.
  • QuerySet's OR operation is documented as it is not a commutative operation since the query of the qs1 | qs2 and qs2 | qs1 might differ, even though the results are expected to be same.

Comment thread django/db/models/sql/query.py Resolved
@github-actions

Copy link
Copy Markdown

Hello @omerfarukabaci! Thank you for your contribution 💪

As it's your first contribution be sure to check out the patch review checklist.

If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!

If you have any design or process questions then you can ask in the Django forum.

Welcome aboard ⛵️!

Comment thread tests/queries/models.py Resolved
Comment thread django/db/models/sql/query.py Resolved
@omerfarukabaci
omerfarukabaci changed the base branch from main to stable/0.90.x November 25, 2021 18:24
@omerfarukabaci
omerfarukabaci changed the base branch from stable/0.90.x to main November 25, 2021 18:25
@omerfarukabaci

Copy link
Copy Markdown
Contributor Author

@charettes Thanks for your review, I hope I understand your point correctly. I have made the related changes, ready to review.

Comment thread django/db/models/sql/query.py Resolved

@felixxm felixxm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omerfarukabaci Thanks for updates 👍

Comment thread tests/queries/tests.py Resolved
Comment thread django/db/models/sql/query.py Resolved
Comment thread django/db/models/sql/query.py Resolved
Comment thread tests/queries/tests.py Resolved
Comment thread tests/queries/models.py Resolved
@omerfarukabaci

Copy link
Copy Markdown
Contributor Author

You're welcome @felixxm! I have made the requested changes. 🚀

@felixxm felixxm changed the title Fixed #33319 - Query.change_aliases raises an AssertionError Fixed #33319 -- Fixed crash when combining with the | operator querysets with aliases that conflict. Dec 8, 2021

@felixxm felixxm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omerfarukabaci Thanks 👍 Welcome aboard ⛵

I pushed small edits and reorganized commits.

@omerfarukabaci

omerfarukabaci commented Dec 8, 2021 •

Copy link
Copy Markdown
Contributor Author

It's flattering to contribute to this great package, thank you guys a lot for your help! 🚀

@carltongibson carltongibson left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks.

I've just left a few possible suggestions on the comments/docs changes. ("suggestions" only :)

👍

Comment thread docs/ref/models/querysets.txt Resolved
Comment thread django/db/models/sql/query.py Resolved
Comment thread django/db/models/sql/query.py Resolved
Comment thread django/db/models/sql/query.py Resolved
@felixxm

felixxm commented Dec 8, 2021

Copy link
Copy Markdown
Member

@carltongibson Thanks 👍 I included your suggestions.

Comment thread docs/ref/models/querysets.txt Resolved
@felixxm
felixxm merged commit 81739a4 into django:main Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants