Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed #35099 -- Prevented mutating queryset when combining with & and | operators. #17829

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Fixed #35099 -- Prevented mutating queryset when combining with & and | operators. #17829

merged 1 commit into from
Feb 7, 2024

Conversation

Hisham-Pak
Copy link
Contributor

Thanks to Alan for reporting this issue.

Ticket: https://code.djangoproject.com/ticket/35099

@Hisham-Pak
Copy link
Contributor Author

Unrelated CI failure

@felixxm felixxm changed the title Fixed #35099 -- Combining QuerySets with "|" or "&" mutates right-hand side Fixed #35099 -- Prevented mutating queryset when combining with & and | operators. Feb 7, 2024
felixxm
felixxm reviewed Feb 7, 2024
View reviewed changes
tests/queries/tests.py Outdated Show resolved Hide resolved
felixxm
felixxm reviewed Feb 7, 2024
View reviewed changes
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.

@Hisham-Pak Thanks 👍 I pushed small edits to tests.

@felixxm felixxm force-pushed the ticket_35099 branch 2 times, most recently from 9c8521e to 3b00fa6 Compare February 7, 2024 10:54
@Hisham-Pak
Copy link
Contributor Author

Hisham-Pak commented Feb 7, 2024 •
edited

@felixxm Thanks for the review. I think assertQuerySetEqual would've passed too. But no need :)

@Hisham-Pak
Copy link
Contributor Author

I remember postgres and oracle failures were because these queries behaved differently on different backends. So shouldn't these asserts be replaced with more dynamic checks?

@felixxm
Copy link
Member

felixxm commented Feb 7, 2024

I remember postgres and oracle failures were because these queries behaved differently on different backends. So shouldn't these asserts be replaced with more dynamic checks?

This has nothing to do with backend. Queryset in tests doesn't make much sense:

        authors_with_extrainfo = Author.objects.filter(
            Exists(ExtraInfo.objects.filter(id=OuterRef("id")))
        )

because you compare IDs of different objects. I will correct it.

@Hisham-Pak @felixxm
Fixed #35099 -- Prevented mutating queryset when combining with & and…
d79fba7 
… | operators.

Thanks Alan for the report.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm felixxm merged commit d79fba7 into django:main Feb 7, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixxm felixxm felixxm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Hisham-Pak @felixxm