Page MenuHomeSoftware Heritage

models: add required on_delete kwarg to ForeignKey
ClosedPublic

Authored by zack on Tue, Sep 24, 11:05 AM.

Details

Summary

courtesy of mypy

Diff Detail

Repository
rDDEP swh-deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

zack created this revision.Tue, Sep 24, 11:05 AM
ardumont requested changes to this revision.EditedTue, Sep 24, 4:48 PM
ardumont added a subscriber: ardumont.

I'm not sure what this does.

Also, when modifying the models, you need to call a routine which generates the migration scripts (corresponding to the change you made in the model).

To sum up [1]:

make db-prepare

That migration script should be commited as well.

It's dev executed with make db-migrate (in production it's executed though django tools).

[1] https://docs.softwareheritage.org/devel/swh-deposit/dev-info.html#schema

[2] https://docs.softwareheritage.org/devel/swh-deposit/sys-info.html#migrate-bootstrap-the-db-schema

This revision now requires changes to proceed.Tue, Sep 24, 4:48 PM
ardumont added a comment.EditedTue, Sep 24, 4:56 PM

I'm not sure what this does.

From [1] Prevent deletion of the referenced object by raising ProtectedError, a subclass of django.db.IntegrityError.

ok then ;)

There is still the migration part to do though.

[1] https://docs.djangoproject.com/en/2.2/ref/models/fields/#django.db.models.PROTECT

zack added a comment.Tue, Sep 24, 5:20 PM

Also, when modifying the models, you need to call a routine which generates the migration scripts (corresponding to the change you made in the model).

Sure, I'll do that, but first I need validation of what you (or anyone else who is more familiar than me with the deposit constraints) want to happen when this referential integrity constraint is violated—specifically, when (and if) the parent deposit of a given deposit is removed from the DB.

What I've picked will make the ORM refuse the deletion, which is the safest option. There is also the option of "doing nothing", which will still make things explode at the DB level (because we use a decent DB). Or to cascade, deleting also all children records.

Let me know what you prefer, and I'll update the diff to include the migration script.

Sure, I'll do that, but first I need validation of what you (or anyone else who is more familiar than me with the deposit constraints) want to happen when this referential integrity constraint is violated—specifically, when (and if) the parent deposit of a given deposit is removed from the DB.

To which i'm ok with ;) [1]

[1] D2031#47038

zack updated this revision to Diff 6827.Wed, Sep 25, 11:07 AM
  • models: add migration to on_delete=protect
ardumont accepted this revision.Wed, Sep 25, 6:07 PM
This revision is now accepted and ready to land.Wed, Sep 25, 6:07 PM
This revision was automatically updated to reflect the committed changes.