#7539 (ON DELETE support) aka ORM-09 - A call for feedback
Johannes Dollinger
work, that solves a few remaining issues and should be fully backwards
compatible. I haven't painted the bikeshed yet, so the API still is
`on_delete=CASCADE | SET_NONE | SET_DEFAULT | PROTECT | DO_NOTHING |
SET(value)`. Two minor additions:
`DO_NOTHING` does nothing. Let the db handle it or resolve the
dependency in pre_delete (see: #10829 [2])
`SET(value)` sets the foreign key to an arbitrary value. `value`
may
be a callable, `SET(None)` is equivalent to `SET_NULL`.
To make `on_delete` work on m2m intermediary models
`DeleteQuery.delete_batch_related()` had to go. Intermediary models
now use (almost) the same related-objects-collection code path as
every other model (thanks to m2m-refactor). Because that would have
lead to lots of SELECT queries for related objects, I refactored the
collection algorithm to collect batches of objects instead of
individual objects. That reduced the overhead to
`#INTERMEDIARY_INSTANCES / GET_ITERATOR_CHUNK_SIZE` queries. This
refactoring has a nice side-effect: Given the following code
class A(models.Model): pass
class B(models.Model): a = models.ForeignKey(A)
class C(models.Model): b = models.ForeignKey(B)
a = A.objects.create()
for i in xrange(100): B.objects.create(a=a)
a.delete()
the `delete()` call results in 103 queries with trunk, and only 4
queries with my patch applied.
Finally, collecting related objects for auto_created intermediary
models is short-circuited to avoid the extra SELECTs completely. The
same could be done for any model that has no related objects, if we
didn't need the instances to send signals (Someday/Maybe:
Meta.send_signals = bool or tuple).
Since the constants used for `on_delete` are just callables, it's
possible to do any kind of calculation to decide what should happen to
related instances, e.g.:
def delete_or_setnull(collector, field, objects):
setnull = []
cascade = []
for obj in objects:
if can_delete(obj):
delete.append(obj)
else:
setnull.append(obj)
SET_NULL(collector, field, setnull)
CASCADE(collector, field, cascade)
fk = ForeignKey(To, on_delete=delete_or_setnull, null=True)
This should probably not be documented at first, but it would be a
nice feature once it's clear the on_delete handler signature will
remain stable.
FIXME:
* I'd like to introduce `DatabaseFeatures.can_defer_constraint_checks`
to disable nulling out foreign keys when it's not necessary. This
would save a couple of UPDATE queries.
* There are ugly contrib.contenttypes imports in
`DeleteQuery.delete_batch_related()`. I left all contenttypes related
code there (just renamed the method, it's still called). Someday/Maybe
this could be removed and handled as a custom `on_delete` argument on
GenericForeignKey.
* There are no docs.
Any feedback welcome.
@glassfordm: If you're still working on this patch, I'd like to hear
what you think (and get those tests you mentioned). I'm sorry I
somewhat hijacked your work.
__
Johannes
[1] http://code.djangoproject.com/ticket/7539
[2] http://code.djangoproject.com/ticket/10829
Michael Glassford
I haven't had a chance to look at the patch yet, but what you describe
here sounds good. I don't have any problem with you "hijacking" the work.
Did your patch deal at all with the unit tests in my patch that
intentionally failed to expose things that weren't working right yet?
Mike
Johannes Dollinger
Am 10.11.2009 um 17:22 schrieb Michael Glassford:
>
> I haven't had a chance to look at the patch yet, but what you describe
> here sounds good. I don't have any problem with you "hijacking" the
> work.
>
> Did your patch deal at all with the unit tests in my patch that
> intentionally failed to expose things that weren't working right yet?
Only the first of your patches contains tests. But the ON DELETE
related tests in this patch that do not test database-level support
should be covered by my tests in modeltests/on_delete/.
Which of your tests failed?
__
Johannes
Michael Glassford
>
> Am 10.11.2009 um 17:22 schrieb Michael Glassford:
>
>> I haven't had a chance to look at the patch yet, but what you describe
>> here sounds good. I don't have any problem with you "hijacking" the
>> work.
>>
>> Did your patch deal at all with the unit tests in my patch that
>> intentionally failed to expose things that weren't working right yet?
>
> Only the first of your patches contains tests.
Oops, right you are. The unit tests were unintentionally omitted. I've
uploaded a new patch which is the pretty much the same as my previous
patch but with my unit tests included (although comparing the two now I
see a few other differences, probably because I also updated to the
latest code in SVN).
> But the ON DELETE
> related tests in this patch that do not test database-level support
> should be covered by my tests in modeltests/on_delete/.
> Which of your tests failed?
The tests that fail are in tests/modeltests/on_delete_django/tests.py.
They have names like test_issue_1a, test_issue_1b, test_issue_2a,
test_issue_2b, etc. The comments on the tests indicate the expected
result. Once the issues they test for are fixed, they should all pass.
Mike
Johannes Dollinger
(harmless) failing tests:
- sanity checks for SET_NULL on fields that are not nullable or
SET_DEFAULT on fields that have no default value. I moved the code to
model validation (and added tests there), so I just removed these tests.
- you assumed SET_NULL as the default behavior for nullable FKs. That
would be backwards incompatible. I changed the default to CASCADE, so
I had to update related tests.
The remaining failing tests are test_issue_2a, -_2b, -_2d, -_2e. They
test if related object caches are updated/cleared after `delete()`,
e.g.:
data = Data.objects.create(data=1)
m = M.objects.create(fk=data) # fk is a nullable ForeignKey
data.delete()
self.assertEqual(None, m.fk) #Fails
I don't think this is required. #17[1] was just rejected in the 1.2
voting process, and without identity mapping (or a global object
collection or excessive use of signals) you would not be able to cover
all use-cases. If you called Data.objects.all().delete() instead of
data.delete() you'd still have outdated related object caches. Or if
you had obtained m through M.objects.get(..). Or ..
I'm aware that django currently *does* null out PKs of deleted objects
as well as nullable references to those objects (which will only be
present in objects that have themselves been deleted). But it's
limited to instances reachable from Model.delete() via reverse one-to-
one descriptors and instances passed to signal handlers.
And finally: it's neither documented nor tested (you can remove the
relavant four lines from delete_objects() without breaking existing
tests).
My patch actually changes the behavior of Model.delete() in this
regard: reverse one-to-one caches are not updated.
I'd rather see model instances as a "checkout" from the db that can
get out of sync. Are there any deeper problems with this approach?
[1] http://code.djangoproject.com/ticket/17
__
Johannes
Johannes Dollinger
Since it's a non-trivial patch and there has been (almost) no
feedback, is it save to assume that #7539 is not in scope for 1.2 ?
> You received this message because you are subscribed to the Google
> Groups "Django developers" group.
> To post to this group, send email to django-
> devel...@googlegroups.com
> To unsubscribe from this group, send email to django-develop...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/django-developers?hl=en
> -~----------~----~----~----~------~----~------~--~---
>
Russell Keith-Magee
<johannes....@einfallsreich.net> wrote:
> Ping.
>
> Since it's a non-trivial patch and there has been (almost) no
> feedback, is it save to assume that #7539 is not in scope for 1.2 ?
the high priority list, #7539/ORM-09 was marked as low priority.
Please don't take offense - your work is definitely appreciated. Keep
the patch warm, and make sure you're ready to go when the call for
proposals for 1.3 comes up.
Yours,
Russ Magee %-)