My idea was to fix this in a very straight way - change ForeignKey behavior (the piece of code that generates many_set method) - so it doesn't take into account null.
Let me explain a bit more. If foreign key is null - then there's no link between Many and One object. So it many_set should return empty set disregarding DB state. Using object state (saved/not saved) makes this thing much more complicated and have unpredicted not covered areas. If user changed object PK and didn't save that into DB - that's his decision, we should not keep the state. Another edge case is null PK (user used custom field as PK) - that is simply covered by initial code, but not by object state. On 16 дек, 17:47, Luke Plant <l.plant...@cantab.net> wrote: > 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 tohttp://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.
