On Thu, 2010-12-16 at 10:56 +0100, Roald de Vries wrote: > Now if I do: > > one = One() > > ... then: > > one.many_set.all() > > ... returns all Many's with 'self.one_id == None'. > > From an implementational (SQL) standpoint, this seems logical, but > from an OO standpoint, it surely doesn't. I would expect an empty > queryset as a result, or maybe even some exception. Is the current > behavior a (conscious) choice/feature, or a bug?
This is related to http://code.djangoproject.com/ticket/14615 - see Gabriel's comments. It is basically a mistake to attempt to do 'one.many_set.all()' when 'one' hasn't been saved. We should either leave it as it is (it is the developers fault), or raise an exception. For the latter, I think we should use instance._state.adding to reliably detect whether a PK has been set or not. This could cause some incompatibilities: in the unusual case of creating an object, setting a primary key manually to some value that you know to be in the database, not saving the object, and then using the object to do queries, you will get exceptions where you didn't before. I think we can regard that workflow as broken, because: 1) there are other places where using model instances in similar ways will currently break - the places where we rely on instance._state.adding to be correct. 2) there are more obvious ways to do queries. 3) the current behaviour is inconsistent with ManyToMany fields - they already do the checks that ForeignKey lookups are not doing. There is in fact one test in the test suite that breaks if we change this - it does Poll().choice_set.all() and expects an empty set. The reason it does this is to implicitly test null queries, but that sounds like a broken test to me (testing implicit behaviour rather than explicit). I've made a patch and attached to the ticket. It required one fix to the existing tests in the test suite (in the null_queries tests), the rest are additions. While I was there, I fixed _get_next_or_previous_by_FIELD to use self._state.adding, rather than 'not self.pk', for the similar check that it does. I also fixed OneToOne fields, and updated ManyToMany lookups to check _state.adding rather than check the PK field. If people agree that the existing behaviour is a bug and that workflows that rely on it are broken, I'll go ahead and commit. Russell closed the bug as WONTFIX, but I think based on a misunderstanding (I misunderstood the original report in the same way). Luke -- "Outside of a dog, a book is a man's best friend... inside of a dog, it's too dark to read." Luke Plant || http://lukeplant.me.uk/ -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-develop...@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
