Opened 11 years ago

Closed 2 years ago

Last modified 2 months ago

#19580 closed Cleanup/optimization (fixed)

Unify reverse foreign key and m2m unsaved model querying

Reported by: Anssi Kääriäinen Owned by: raydeal
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: olivier.tabone@…, Matt Westcott Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Currently when querying unsaved reverse relations the behavior differs.

Using model:

class Foo(models.Model):
    fk = models.ForeignKey('self', related_name='fk_rev')
    m2m = models.ManyToManyField('self')

and test case:

print(Foo().fk_rev.all())
print(Foo().m2m.all())

We get [] from the first filter, but an error

ValueError: "<Foo: Foo object>" needs to have a value for field "from_foo" before this many-to-many relationship can be used.

from the second filter.

So, m2m fields can't be filtered if the object isn't saved, but reverse fk fields can be filtered.

There is a (slightly stale) patch for #17541 which makes fk fields and m2m fields work consistently. The changes in behavior are:

* Nullable many-to-many and foreign key relations will return an empty
  queryset when the relation field is null. For many-to-many this was
  previously an error (no change for foreign keys).

* Trying to add objects to a foreign key relation when the relation field
  is null is now an error (no change for m2m).

* Trying to use a relation of any kind when the object isn't saved is now
  an error (no change for m2m).

I think we can squeeze these changes in as bug-fixes. These are slight backwards compatibility changes, but to me it seems that almost always the changes will be visible only in code that isn't working as intended. If these are seen as something likely breaking working code, then I don't see any way to make the APIs consistent.

The #17541 patch is available from: https://github.com/akaariai/django/compare/ticket_17541

Change History (57)

comment:1 by Anssi Kääriäinen, 11 years ago

Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed
Type: UncategorizedCleanup/optimization
Version: 1.4master

Reviewing my own ticket as "DDN".

comment:2 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

Yes, it's better to error loudly on operations that can't work because the database didn't generate an id for the object yet.

comment:3 by Tim Graham, 8 years ago

Easy pickings: set
Summary: Unify reverse foreign key and m2m querying behaviorUnify reverse foreign key and m2m unsaved model querying

I think this would allow removing the two lines added in a4c20ae85b40c49e28d1b2227208e4f00d7820df.

Marking as "Easy pickings" since rebasing Anssi's original patch might not be too difficult.

comment:4 by Ketan Bhatt, 8 years ago

Owner: changed from nobody to Ketan Bhatt
Status: newassigned

I will create a new branch, and make the changes made in https://github.com/django/django/compare/master...akaariai:ticket_17541

Will that be correct?

Last edited 8 years ago by Ketan Bhatt (previous) (diff)

comment:5 by Tim Graham, 8 years ago

Yes, you should rebase the branch and update the release notes for 1.11.

comment:6 by Tim Graham, 8 years ago

Owner: Ketan Bhatt removed
Status: assignednew

comment:7 by Michael Scott, 8 years ago

Hi,

I'm interested in contributing to Django and have been advised to start with some of the 'easy pickings' tasks. I've seen it's been unassigned and is still open, is this something I can pick up?

Cheers,
Michael

comment:8 by Andrey Kuzminov, 8 years ago

Owner: set to Andrey Kuzminov
Status: newassigned

comment:9 by Tim Graham, 8 years ago

I left comments for improvement on the rebased PR.

comment:10 by Andrey Kuzminov, 8 years ago

Thanks Tim, I'll add more tests next week

comment:11 by Olivier Tabone, 7 years ago

Cc: olivier.tabone@… added

comment:12 by Olivier Tabone, 7 years ago

added comments in the PR

comment:13 by Tim Graham, 7 years ago

Patch needs improvement: unset

comment:14 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:15 by eadhina, 7 years ago

Hey guys, is this ticket still need a fix ?

comment:16 by John McCann, 7 years ago

Hi there, I was also wondering if further work is needed? From Tim's final comment in the pull request, it seems that most of the changes from Ansi's patch still haven't been updated in master. Thanks

comment:17 by Tim Graham, 7 years ago

Owner: Andrey Kuzminov removed
Status: assignednew

I think someone else could pick this up considering we haven't heard from Andrey in a while.

comment:18 by Lander Kerbey, 7 years ago

Newbie here, but I've begun reading through this problem and would like to take a crack at it. Andrey's pull request seems most of the way there, and if the comments remain valid that gives me a starting point. Thoughts?

comment:19 by Tim Graham, 7 years ago

That's a correct assessment of the situation.

comment:20 by Rajesh Veeranki, 7 years ago

Owner: set to Rajesh Veeranki
Status: newassigned

Please review the latest PR, that is rebased with the master here: https://github.com/django/django/pull/8570
The tests look okay to me. Please suggest any more tests if required. Thanks!

comment:21 by Rajesh Veeranki, 7 years ago

Patch needs improvement: unset

comment:22 by Carlton Gibson, 6 years ago

Patch needs improvement: set

Comments on PR

comment:23 by Rajesh Veeranki, 6 years ago

Patch needs improvement: unset

comment:24 by Carlton Gibson, 6 years ago

Patch needs improvement: set

comment:25 by Robert Švab, 5 years ago

Hi everyone, I'm interested in contributing to Django, so would like to finish this easy pick if needed. From comments on PR I can see there is only few things left, adjustments to tests and docs. Advise me if there is anything else left.

comment:26 by Mariusz Felisiak, 5 years ago

Owner: Rajesh Veeranki removed
Status: assignednew

comment:27 by Ahsan Shafiq, 5 years ago

Owner: set to Ahsan Shafiq
Status: newassigned

comment:28 by Ahsan Shafiq, 4 years ago

Patch needs improvement: unset

I've created a PR to fix this issue. Please review and let me know if any change required. Thanks
https://github.com/django/django/pull/11807

Last edited 4 years ago by Ahsan Shafiq (previous) (diff)

comment:29 by Mariusz Felisiak, 4 years ago

Needs documentation: set
Needs tests: set

comment:30 by Manav Agarwal, 3 years ago

Can I claim this? I think I can move this forward

comment:33 by Pavel Druzhinin, 3 years ago

Owner: set to Pavel Druzhinin

I'd like to try finish this

comment:34 by Pavel Druzhinin, 3 years ago

Need some help with the ticket. PR: https://github.com/django/django/pull/13784.
Thanks!

comment:35 by Pavel Druzhinin, 3 years ago

Rebased the previous PR https://github.com/django/django/pull/11807.
I would like to get some clarification on this old ticket, took it under easy-pickings tag.

https://github.com/django/django/pull/11807#pullrequestreview-344544165 - this is the last comment from where I started to search next steps to complete the ticket.
It leads to the undone comments:

  1. https://github.com/django/django/pull/8570#pullrequestreview-113177526

Should I move all tests from m2m_through_regress to many_to_many or some particular tests?

  1. https://github.com/django/django/pull/8570#pullrequestreview-113506649

I see prepared changelogs in comment https://github.com/django/django/pull/11807#issuecomment-612404721, but don't know where to place them in codebase. Could you explain?

Thanks!

comment:36 by Pavel Druzhinin, 3 years ago

Needs documentation: unset
Needs tests: unset

comment:37 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

Thanks for this patch, however it's not ready for review. Please fix tests and isort.

in reply to:  37 ; comment:38 by Pavel Druzhinin, 3 years ago

Replying to Mariusz Felisiak:

Thanks for this patch, however it's not ready for review. Please fix tests and isort.

Hello, it seems I need help to fix this error. I'm not sure code from previous contributor is correct and I don't fully understand the subject of this bug (m2m relations).
What is better to do: remove label easy pickings and try another bug or someone could help me with fixing this bug?

P.S. Please, don't think for nagging. I really don't want to drop the ticket silently and want to continue to contribute at the same time. Thanks in advance with your advice how best to proceed.

comment:39 by raydeal, 2 years ago

Owner: set to raydeal

I would like to look at code and try to improve the patch.

in reply to:  38 ; comment:40 by raydeal, 2 years ago

Replying to Pavel Druzhinin:

Replying to Mariusz Felisiak:

Thanks for this patch, however it's not ready for review. Please fix tests and isort.

Hello, it seems I need help to fix this error. I'm not sure code from previous contributor is correct and I don't fully understand the subject of this bug (m2m relations).
What is better to do: remove label easy pickings and try another bug or someone could help me with fixing this bug?

P.S. Please, don't think for nagging. I really don't want to drop the ticket silently and want to continue to contribute at the same time. Thanks in advance with your advice how best to proceed.

Would you like to carry on with the ticket? I can help you. After reading comments and code my understanding is to make FK reverse working the same way as m2m when id=None, what is done in code, but message is a bit different, it would be good to have similar message in both cases. It will require amend m2m tests as well after update with current django version.

Last edited 2 years ago by raydeal (previous) (diff)

in reply to:  40 ; comment:41 by Asif Saifuddin Auvi, 2 years ago

Replying to raydeal:

Replying to Pavel Druzhinin:

Replying to Mariusz Felisiak:

Thanks for this patch, however it's not ready for review. Please fix tests and isort.

Hello, it seems I need help to fix this error. I'm not sure code from previous contributor is correct and I don't fully understand the subject of this bug (m2m relations).
What is better to do: remove label easy pickings and try another bug or someone could help me with fixing this bug?

P.S. Please, don't think for nagging. I really don't want to drop the ticket silently and want to continue to contribute at the same time. Thanks in advance with your advice how best to proceed.

Would you like to carry on with the ticket? I can help you. After reading comments and code my understanding is to make FK reverse working the same way as m2m when id=None, what is done in code, but message is a bit different, it would be good to have similar message in both cases. It will require amend m2m tests as well after update with current django version.

you should take over the ticket and create a new PR as it seems he is not able to fix the issue

in reply to:  41 comment:42 by Pavel Druzhinin, 2 years ago

Replying to Asif Saifuddin Auvi:

Replying to raydeal:

Replying to Pavel Druzhinin:

Replying to Mariusz Felisiak:

Thanks for this patch, however it's not ready for review. Please fix tests and isort.

Hello, it seems I need help to fix this error. I'm not sure code from previous contributor is correct and I don't fully understand the subject of this bug (m2m relations).
What is better to do: remove label easy pickings and try another bug or someone could help me with fixing this bug?

P.S. Please, don't think for nagging. I really don't want to drop the ticket silently and want to continue to contribute at the same time. Thanks in advance with your advice how best to proceed.

Would you like to carry on with the ticket? I can help you. After reading comments and code my understanding is to make FK reverse working the same way as m2m when id=None, what is done in code, but message is a bit different, it would be good to have similar message in both cases. It will require amend m2m tests as well after update with current django version.

you should take over the ticket and create a new PR as it seems he is not able to fix the issue

I agree with this proposal, you could change an assignee. Thanks!

comment:43 by Ayush Joshi, 2 years ago

raydeal if you are currently unable to work on this ticket then you can assign it to me.

in reply to:  43 comment:44 by raydeal, 2 years ago

Replying to Ayush Joshi:

raydeal if you are currently unable to work on this ticket then you can assign it to me.

I am working on the ticket

comment:45 by raydeal, 2 years ago

Patch needs improvement: unset

comment:46 by Mariusz Felisiak, 2 years ago

Needs tests: set
Patch needs improvement: set

comment:47 by raydeal, 2 years ago

My patch is implemented using different approach than previous. It changes behaviour of FK to be the same as M2M.

I went through discussion in this and #17541 ticket and PR for them and analysed examples.
This information in the ticket
" There is a (slightly stale) patch for #17541 which makes fk fields and m2m fields work consistently."
might be true 9 years ago, but now it is not consistent with M2M.

I have also tested previous patch (https://github.com/django/django/pull/13784) locally.
I couldn't find correct rules because M2M worked as always, only changed behaviour of FK. When object is not saved it raises ValueError, when saved but related value is None returns <QuerySet []>, which is not consistent for me. Why?
Base on doubt from #17541 https://code.djangoproject.com/ticket/17541#comment:8 I asked myself: What is the difference between not saved object with id (pk) refrerenced from other object and saved object with field containing None value referenced by other object, through FK both, from the relation point of view?
There is no difference - both of them have None value and making related query in both cases doesn't make sens.

So finally I came to the conclusion that what M2M is doing is correct - in both cases it raises error - and it meet some of The Zen of Python rules, I think.

Last edited 2 years ago by raydeal (previous) (diff)

comment:48 by raydeal, 2 years ago

Needs tests: unset
Patch needs improvement: unset

in reply to:  47 ; comment:49 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

Replying to raydeal:

I couldn't find correct rules because M2M worked as always, only changed behaviour of FK. When object is not saved it raises ValueError, when saved but related value is None returns <QuerySet []>, which is not consistent for me. Why?
Base on doubt from #17541 https://code.djangoproject.com/ticket/17541#comment:8 I asked myself: What is the difference between not saved object with id (pk) refrerenced from other object and saved object with field containing None value referenced by other object, through FK both, from the relation point of view?
There is no difference - both of them have None value and making related query in both cases doesn't make sens.

There is a huge difference for me. It's doesn't matter which field is pointed by m2m field, that's not crucial. The most important thing is that Foo() doesn't exist in the database so any related query makes no sense, and IMO we should raise an exception only in this particular case. You can start a discussion on DevelopersMailingList if you don't agree, where you'll reach a wider audience and see what other think.

Moreover proposed change is not a part of the ticket description, it's backward incompatible, and was not accepted per se.

in reply to:  49 comment:50 by raydeal, 2 years ago

Replying to Mariusz Felisiak:
Right, now I better understand what is expected solution. I am going to review again previous patch and change this one.

comment:51 by raydeal, 2 years ago

Patch needs improvement: unset

comment:52 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:53 by raydeal, 2 years ago

Patch needs improvement: unset

comment:54 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:55 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 7ba6ebe9:

Fixed #19580 -- Unified behavior of reverse foreign key and many-to-many relations for unsaved instances.

comment:56 by Matt Westcott, 2 years ago

Cc: Matt Westcott added

Ran across this while bringing Wagtail up to date with Django main. I appreciate that accessing a reverse-FK relation on an unsaved instance is undefined behaviour, so this isn't technically a breaking change - however, I suspect there could be a lot of code in the wild that's doing this unwittingly, in which case this change might be more disruptive than expected.

For example, I had to make this fix: https://github.com/wagtail/wagtail/pull/8028/commits/9e19bf63dbb698a4f57b131b6f8ae92bab7c2606 - where I'd implemented something approximating an InlineFormSet, but neglected to make a special case for the 'create' view, and so it ends up querying an unsaved instance to arrive at the initial empty formset. I imagine this will be a fairly common gotcha in user code.

(for the record, Wagtail also does much more esoteric things with faking relations on in-memory objects to support previews and saving drafts, but I'm willing to take the hit for that :-) )

Should we consider making this a deprecation warning for now, and adding the strict validation in Django 5.x instead? (Admittedly, this isn't the sort of thing you can write a startup check for, so it may be that a deprecation warning would only help for apps that have good test coverage anyhow.)

comment:57 by Mariusz Felisiak, 2 years ago

Should we consider making this a deprecation warning for now, and adding the strict validation in Django 5.x instead? (Admittedly, this isn't the sort of thing you can write a startup check for, so it may be that a deprecation warning would only help for apps that have good test coverage anyhow.)

IMO deprecation is unnecessary, we raise an exception so it's not something can be easily missed. Also, this is undocumented behavior and as far as I'm aware it should be quite rare. If you think deprecation is necessary, please first start a discussion on the DevelopersMailingList, where you'll reach a wider audience and see what other think.

comment:58 by Alex Matos, 5 months ago

I work on a large Django codebase and while upgrading it from version 3.2 to 4.2 a lot of code broke due to this change, where code that was once returning an empty queryset, now raises a ValueError. In my opinion, this change should be fully reverted. The partial update from #33952 is not enough. I'm pretty sure I'm not alone in this situation. One of Django's core strengths has always been its stability and breaking user code without even a deprecation warning should be avoided at all costs.

comment:59 by Adam Sołtysik, 2 months ago

I agree with the above. Why does something like this (where self is an unsaved User instance)

Book.objects.filter(author=self).exists()

display a proper RemovedInDjango50Warning, while equivalent

self.books.exists()

suddenly crashes? This is totally unexpected.

Note: See TracTickets for help on using tickets.
Back to Top