Opened 4 years ago

Last modified 5 days ago

#19580 assigned Cleanup/optimization

Unify reverse foreign key and m2m unsaved model querying

Reported by: akaariai Owned by: akuzminov
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (10)

comment:1 Changed 4 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed
  • Type changed from Uncategorized to Cleanup/optimization
  • Version changed from 1.4 to master

Reviewing my own ticket as "DDN".

comment:2 Changed 3 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 3 months ago by timgraham

  • Easy pickings set
  • Summary changed from Unify reverse foreign key and m2m querying behavior to Unify 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 Changed 3 months ago by ketanbhatt

  • Owner changed from nobody to ketanbhatt
  • Status changed from new to assigned

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 3 months ago by ketanbhatt (previous) (diff)

comment:5 Changed 5 weeks ago by timgraham

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

comment:6 Changed 2 weeks ago by timgraham

  • Owner ketanbhatt deleted
  • Status changed from assigned to new

comment:7 Changed 2 weeks ago by mscott250

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 Changed 2 weeks ago by akuzminov

  • Owner set to akuzminov
  • Status changed from new to assigned

comment:9 Changed 2 weeks ago by timgraham

I left comments for improvement on the rebased PR.

comment:10 Changed 5 days ago by akuzminov

Thanks Tim, I'll add more tests next week

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