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.

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

Fixed #34421 -- Fixed QuerySet.update() on querysets in descending order by annotations. #16657

Merged

Conversation

HB6H057
Copy link
Contributor

@HB6H057 HB6H057 commented Mar 16, 2023

I often use admin to manage data, so I wrote an action to update data in bulk, but sometimes it reports an error

FieldError: Cannot resolve keyword 'message_length' into field. Choices are: message, id, text, x

And the error is reported in descending order, similar to the execution of the following query statement

Model.objects.annotate(message_length=Length('message')).order_by('-message_length').update(text="Can I come on board?")

Through debug found that the programmer ignored the case of descending order, so wrote this patch to fix the problem, see the code for more details.

First submission, so if there are any questions please guide me.

@charettes
Copy link
Member

charettes commented Mar 16, 2023

@felixxm, not sure how we want to treat this one.

It's clearly a case missed by #15747 / ticket-28897 which is part of 4.2 but with and without the patch the test fails with the same signature

django.core.exceptions.FieldError: Cannot resolve keyword 'abs_id' into field. Choices are: foo, foo_id, id, m2m_foo, x

So it's technically not a regression but certainly a bug in newly introduced feature that allows update to be ordered by annotations. cc @David-Wobrock

@felixxm
Copy link
Member

felixxm commented Mar 16, 2023

So it's technically not a regression but certainly a bug in newly introduced feature that allows update to be ordered by annotations. cc @David-Wobrock

ticket-28897 is classified as a bug (not a new feature) and #15747 is a bugfix so I don't think it's a release blocker for Django 4.2 🤔

@felixxm
Copy link
Member

felixxm commented Mar 16, 2023

So it's technically not a regression but certainly a bug in newly introduced feature that allows update to be ordered by annotations. cc @David-Wobrock

ticket-28897 is classified as a bug (not a new feature) and #15747 is a bugfix so I don't think it's a release blocker for Django 4.2 thinking

... but I'm happy to backport it, if we will have a reasonable patch before Monday.

@charettes
Copy link
Member

yeah I'm on the fence as well, whatever works best here.

@felixxm
Copy link
Member

felixxm commented Mar 17, 2023

yeah I'm on the fence as well, whatever works best here.

I noticed that 3ef37a5 was backported to Django 4.1 (in alpha) so it definitely doesn't qualify for a backport. I'd treat it as a separate bug.

@HB6H057 Can you create a new ticket in Trac? All bugfixes require an accepted ticket.

django/db/models/query.py Outdated Show resolved Hide resolved
@felixxm felixxm changed the title Fixed QuerySet.update() on querysets ordered by annotations desc error. Fixed #XXXXX -- Fixed QuerySet.update() on querysets in descending order by annotations. Mar 17, 2023
@HB6H057 HB6H057 changed the title Fixed #XXXXX -- Fixed QuerySet.update() on querysets in descending order by annotations. Fixed #34421 -- Fixed QuerySet.update() on querysets in descending order by annotations. Mar 17, 2023
@HB6H057
Copy link
Contributor Author

HB6H057 commented Mar 17, 2023

@felixxm I created a ticket and added the tests to the list of skipped tests, but now I have a little problem with the suggestion. Please see if there are any other problems, thanks.

@felixxm felixxm force-pushed the fix-update-when-ordering-by-an-aggregate-desc branch from fc780a3 to 99b2d33 Compare March 17, 2023 19:20
@felixxm
Copy link
Member

felixxm commented Mar 17, 2023

@HB6H057 Thanks for updates 👍 Welcome aboard ⛵

I pushed small edits.

@HB6H057
Copy link
Contributor Author

HB6H057 commented Mar 17, 2023

@felixxm Hi, it occurred to me that you used the new python 3.9 feature removeprefix in your modified code, and the django documentation shows that version 4.x supports python 3.8, so is that okay?
image

@felixxm
Copy link
Member

felixxm commented Mar 18, 2023

@felixxm Hi, it occurred to me that you used the new python 3.9 feature removeprefix in your modified code, and the django documentation shows that version 4.x supports python 3.8, so is that okay? image

On the main branch we're developing Django 5.0 which support Python 3.10+.

@felixxm felixxm force-pushed the fix-update-when-ordering-by-an-aggregate-desc branch from 99b2d33 to 2ffa815 Compare March 18, 2023 12:20
@felixxm felixxm merged commit 2ffa815 into django:main Mar 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants