Skip to content
/ django Public

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

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

Fixed #33319 -- Fixed crash when combining with the | operator querysets with aliases that conflict.#15128
felixxm merged 3 commits intodjango:mainfrom
omerfarukabaci:ticket_33319

Conversation

@omerfarukabaci
Copy link
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.

@github-actions
Copy link

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 ⛵️!

@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
Contributor Author

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

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@omerfarukabaci Thanks for updates 👍

@omerfarukabaci
Copy link
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
Copy link
Member

@felixxm felixxm left a comment

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
Copy link
Contributor Author

omerfarukabaci commented Dec 8, 2021

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

Copy link
Member

@carltongibson carltongibson left a comment

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 :)

👍

@felixxm
Copy link
Member

felixxm commented Dec 8, 2021

@carltongibson Thanks 👍 I included your suggestions.

@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