jazzband / django-polymorphic

Improved Django model inheritance with automatic downcasting
https://django-polymorphic.readthedocs.io
Other
1.65k stars 279 forks source link

Creating polymorphic instances in migrations results in invalid model instances (polymorphic_ctype_id is null) #388

Open atleta opened 5 years ago

atleta commented 5 years ago

When you create an instance of a polymorphic model in a (data) migration, the polymorphic_ctype_id will be left null. The reason is that this field is set in PolymorphicModel.pre_save_polymorphic which is called from save. However, save is never called when running migrations, because in migrations we don't use the actual model classes but the 'historical models' maintained by the migrations engine. As the documentation says: "Because it’s impossible to serialize arbitrary Python code, these historical models will not have any custom methods that you have defined. " This includes the custom save (and pre_save_polymorphic) methods.

The result is that naively creating a polymorphic model instance from migrations will create instances without a valid polymorphic_ctype FK which will cause problems down the road when you try to access these instances. And this is pretty evil as it won't necessarily be caught by tests.

This has been reported before (see https://github.com/django-polymorphic/django-polymorphic/issues/197 ), but closed due to a misunderstanding probably with a reference to the documentation explaining how to migrate existing objects. But this is a distinct problem.

Now besides this being a documentation problem (i.e. undocumented), also raises the question if polymorphic_ctype should be non-nullable. (I don't know what's the decision behind allowing null values, maybe there is a good reason, but getting any instance with a null polymorphic_ctype will fail with an exception which hints that it shouldn't be allowed.)

Setting polymorphic_ctype non-nullable would at least prevent creating invalid data. As far as I can see, the only solution is to manually set the polymorphic_ctype as mentioned in the documentation for migrating existing models. (But note, that the documentation is wrong about how to do that. I'll open another issue for that.)

Exchizz commented 3 years ago

Thanks for sharing this - i just couldn't figure out why it wasn't working...

jeking3 commented 2 years ago

Also see #148.

jeking3 commented 2 years ago

Regarding documentation being wrong, that was resolved in #389.

nikoder commented 1 year ago

Note that there is also the related problem of deleting polymorphic instances in a data migration: I think in this case, the parent(s) won't be deleted, essentially leaving them around with an also invalid, though non-null, polymorphic_ctype. This leads to the same kinds of problems down the road.

Not sure if there should be a separate issue for this.

I also stumbled across the problem of polymorphic behaviour not being applied during (data) migrations the unpleasant way and have been looking for a solution to make sure the expected behaviour is applied since. So far, no luck for something general, but if it were easy, then I guess it would already be part of the library.

I feel like it would be nice to have a hard to miss warning about data migrations in the documentation somewhere, though, given the time consuming surprises I've had.

nikoder commented 1 year ago

In case it's useful to someone, these are the best workarounds I've found for data migrations:

Creation

Just make sure to set the poylmorphic_ctype explicitly on all model instances created. As is documented, the correct value can be found by adding:

ContentType = apps.get_model('contenttypes', 'ContentType')
new_ct = ContentType.objects.get_for_model(MyModel)
Deletion

I delete the base model instead of the child and rely on cascading down the parent pointers to delete all children. Finding the base class seems to be accomplished most flexibly by checking the model which defined the polymorphic_ctype field:

ModelBaseClass = MyChildModel._meta.get_field("polymorphic_ctype").model

Caveats:

atleta commented 1 year ago

Well, since you do know when writing the migration what the base class is, you can refer to it directly :) . Actually that is the preferred way since the code can change and you don't know what MyChildModel._meta.get_field("polymorphic_ctype").model will be at an unknown time in the furure. (Though it's unlikely that the base class will change for a model, it's not out of question.) This is a generic principle to keep in mind when writing Django migrations. Even if it's just your app and not a reusable, published one. It can bite e.g. when you have to setup a new environment (e.g. because you add a new dev to the team) and run old migrations you have long forgotten about.

Also, the pk of the child is set to be a FK on the parent's PK so they are guaranteed to be the same and it also guarantees that deleting the parent instance will cascade the delete (or that the deletion will fail if for some reason the tables weren't created properly).