On Thu, Oct 7, 2010 at 5:30 PM, George Sakkis <george.sak...@gmail.com> wrote: > There are at least three open tickets related to OneToOneFields > (#10227, #14043, #14368) that, even if deemed invalid, hint at lack of > adequate documentation. After reading the docs on OneToOneField, I > don't think one can easily answer the following questions: > > - It is mentioned that multi-table inheritance in implemented with an > implicit O2O relation from the child to the parent model, but O2O is > also useful for cases where neither model "extends" (in the OO sense) > the other. What are the implications of model A linking to model B > versus the inverse ? > - Does A.save() propagate to the linked B instance (if any) or vice > versa ? > - Does A.delete() propagate to the linked B instance (if any) or vice > versa ? > - Is "A.link_to_B = B" equivalent to "B.link_to_A = A" ? > - Is "A.link_to_B = None" or "B.link_to_A = None" valid ? > - Are the assignments above done exclusively in memory or they may hit > the database for reading and/or writing ? > - How does null=True affect the answers to all the above ? > > Having clear answers to these would be a good first step to moving on > with the mentioned tickets and perhaps augmenting the docs.
The problem we have here is that O2O fields are the worst possible case for the leaky abstraction that forms the basis of the ORM. ForeignKeys have the same problems, but they're not quite as acute because they at least *appear* asymmetrical. *In theory* there shouldn't be a difference between which side of the fence you operate upon. However, in practice, there is. One of the two models has to store the column that represents the relation. If A contains the field definition, you need to save the instance of A in order to make a relation change persist. This means that the pure object abstraction gets leaky, and you have to care a little bit about which model holds the physical manifestation of your relation. And pretty much all the questions you ask related to exactly how leaky the abstraction is (or should be). So - the overarching behavior should be that it the ORM looks and feels like you're manipulating a bunch of objects. However, there are practical limitations to that pure vision, and places where the abstraction leaks, and the solution we've implemented is mindful of that fact. Looking at the specific bugs in question: **** #10227 is a bit of a weird one (which is why it's DDN). I can certainly see the argument for why the reverse case should return None rather than an exception. If you consider a O2O field to be a ForeignKey with unique=True (which is essentially how they're implemented), you don't get an exception when there is no objects in your object_set.all(); you get an empty query set. I think there's grounds to say that "None" should be the O2O interpretation of an empty query set, rather than a DoesNotExist exception. The patch as proposed breaks 2 tests in the current suite, but the error occurs in deletion code, so it's possible it's just a case of an incomplete patch, rather than an outright problem. The bigger problem is that the proposed change would change existing API usage. I'm not quite ready to call it backwards-incompatible, because our backwards-compatibility policy documents the fact that we don't consider bugfixes to be a backwards incompatible change, and it could be argued that this is bufix. Providing additional wiggle room: the 'does it return None or raise an exception' doesn't appear to be tested, and it isn't clearly documented. This makes it much easier to argue that this is undocumented and undesigned behavior. The fact that this bug has existed for a long time without being reported suggests that this isn't a high-traffic area of the API, either, which means that the impact footprint of any change will be smaller. However, this isn't an area where only one opinion should matter. I'm not comfortable making a judgement here without hearing the opinion of others. **** #14043 is clearly a bug to me (hence the accepted status). If I had to guess at a cause, I'd say it's either: * The OneToOneField special case not being handled by deletion traversal * The related object cache on the o2o field not being cleared correctly when null is assigned, causing the delete cascade to operate on the older cached object. The 'on delete set null' comment doesn't seem to be related here -- if I've understood the ticket correctly, the relation has been broken by 'become_ghost()', so deletion shouldn't be cascading, regardless of whether the relation implements ON DELETE CASCADE or not. **** #14368 also strikes me as a bug, but it's one that's a little hard to account for without some other changes. In saying bob.soul = None, you then need to save the soul object in order for the change to take effect. That isn't something that is currently done. However, interestingly, it *is* done if you do the analogous operation with a foreign key -- if you assign a queryset to a reverse FK relation, every object in the queryset is modified and saved (and looking at the code, this is something we can *massively* optimize with an update statement). In the interests of removing a wierd inconsistency, I can cer However, this is a situation where pragmatism will need to beat purity. OneToOne fields are also the cornerstone of inheritance, and autosaving OneToOne reverse relations strikes me as something that could backfire in the internals to inheritance. I'd need to see a sample patch that doesn't break the existing test suite before I could make any more firm judgements. **** I hope that sheds some light on the situation. Yours, Russ Magee %-) -- 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.
