on_delete option for foreign keys not picked up by south

[yishai@…]

The on_delete option (new in Django 1.3: ​http://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.ForeignKey.on_delete) available for foreignkey fields is not picked up by South.

[andrew]

This would also possibly impact #752.

[anonymous]

Any news on this? Would really love to see this implemented...

[andrew]

Any news would have been posted here - nothing on this yet, as nobody's submitted a patch and I haven't had the time.

[George Lund <glund@…>]

Fix for many on_delete cases

[George Lund <glund@…>]

Hi there

My patch fixes (I think) most cases, except for where you use SET either with an actual sentinel object, or a callable that returns one. I think the first of these is probably not fixable. It ought to be possible to reference a function, but I haven't managed to make that work yet.

Some proper test cases are really needed, but I wasn't quite sure where to put them or how to go about it.

The issue with referencing a function is that you must have access to the function in the local imports, when the FakeORM is constructed. For functions local to the current models.py, this should be possible. The comments in eval_in_context say:

Drag in the migration module's locals (hopefully including models.py)

But in the migration files South was creating for me, it never included the models.py. This doesn't usually matter, because it adds in all the models we need. But if I am to have access to any local 'sentinel' function, it does need to be there.

And this will naturally fail anyway if the sentinel function in fact needs to be imported from somewhere else, or has meanwhile vanished from the 'current' version of models.py.

For my purposes, I don't need arbitrary SET -- and I'm quite sure that SET_NULL, PROTECT and CASCADE (the default) will be the most common requirements, which I have handled.

Let me know if you'd like any further updates to this patch - thanks

George

[George Lund <glund@…>]

Further patch (on top of last one) to fix default handling, to avoid spurious db changes when switching to a south with this new on_delete handling.

[George Lund <glund@…>]

Working patch for this ticket

[George Lund <glund@…>]

The previous patch actually broke the implementation, so I've added a new patch, which also fixes the modelinspector tests, which would have caught this.

I've spent a long time trying to make a test case for the actual generated ORM itself (i.e. a real test for this issue), but so far failed. I'll post what I managed on the mailing list.

[andrew]

Committed as [9faade77e7b4]. Thanks.

[anonymous]

Not able to use SET to a callable that returns a sentinel object is almost a deal breaker for me. The get_sentinel_object pattern is in the django documentation page and is heavily used in my application. I hope you will reconsider fixing this scenario.

Thanks.

[andrew]

Discussions are ongoing on how to treat SET with a callable - it's impossible to serialise that, so the only other option to either let it pass and then not let you delete things from data migrations.

[glund@…]

The current fix also doesn't support actually changing the database schema (e.g. both SQL Server and InnoDB support ON DELETE SET NULL at the database level). It would be nice if South knew how to make that change (even though Django does the setting to null itself, so that it can support MyISAM etc.).

(This would certainly help my scenario where we have several non-Django apps using the same database.)

So maybe we need one or more further tickets to handle these -- some of which will be more or less tractable problems!

[andrew]

The current rule is that South doesn't set any database settings that Django omits to set - it's very unlikely we'll support triggers/ON DELETE in databases in the near future, as the extra complexity would not be worth it for the tiny number of apps which require that feature.

If you do need it, you can always do it manually by just editing the migration and adding SQL statements with db.execute().

[anonymous]

Rather than raise a ValueError?, can you just issue a warning? Since callable functions (such as get_sentinel_object) does not require any sql changes, I think it's safe to just ignore it. In my current setup, I remove the on_delete arg

 field = models.ForeignKey(..., on_delete=models.SET(get_sentinel_user))

right before calling schemamigration, do the migration for any other changes, and then add the on_delete arg back. The schema migration works as expected.

[glund@…]

The issue is what happens if you make updates (specifically, deletions of related objects) using that ORM in a data migration. The behavior that existed at the time the data migration was created just can't be re-created as it can for other field options.

What might be a good idea would be to create a stub function that raises an error. Or just substitute models.PROTECT, which would have the same effect.

But allowing through a set of models that doesn't function the same under a data migration as under regular Django code might be seen as a dangerous precedent - with sufficient documentation I think a fail-safe solution like the above might be okay, though.

[andrew]

I agree that adding models.PROTECT is probably a decent fallback solution - I can't think of a case where that would cause data loss off the top of my head.