Fix `invalidate_m2o` for polymorphic models

[aletor123]

Now there is problem with polymorphic models and invalidate_m2o. When Model is inherited it's pk field is OneToOne field to parent model. And if there is some other model with FK to inherited model, this FK points to OneToOne field. In this situation getattr(instance, f.field_name) will return model instance of parent model, when we need to get id.

We can fix it using Model._meta.pk.attname. Test case presented. Can you please add this fix to 6.1

[Suor]

First of all, welcome to cacheops and thanks for the PR.

You should note though that multitable inheritance was never really supported by cacheops, in a sense that invalidation is not guaranteed but things should not blow up. Do they? It's not clear. Anyway, even if they don't I am willing to accept patches to improve multitable inheritance support as long as it's not me supporting them :), so we can proceed on that base too.

Some specifics:

• can you use existing models for tests instead of creating new ones? E.g. Media and Movie
• it is better to have a real test with no mocks and no internal calls like invalidate_m2o(), for now I don't understand what is that is broken and also see this test as fragile and hard to support one
• maybe the problem is that OneToOneRel is a subclass of ManyToOneRel? I.e. things may start to work if we restrict invalidate_m2o() strictly to ManyToOneRel. Or alternatively we simply need different handling for these two

Current code with if looks awkward and makes me suspect that it is not covering everything, but maybe I am wrong.

[aletor123]

it is better to have a real test with no mocks and no internal calls like invalidate_m2o(), for now I don't understand what is that is broken and also see this test as fragile and hard to support one

The main idea of this test is to show that getattr(instance, f.field_name) can return model instance instead of id, that is not acceptable situation, because in invalidate_dict json.dumps(obj_dict, default=str) will be called and it will call Model.str method that can contain some operations with already deleted related objects and error will be raised. I added test for this

maybe the problem is that OneToOneRel is a subclass of ManyToOneRel? I.e. things may start to work if we restrict invalidate_m2o() strictly to ManyToOneRel. Or alternatively we simply need different handling for these two

No real problem is that f.field_name is to_field attribute that gets on ForeignKey initialization This attribute points to field that is pk of Model. In situation with multi-table inheritance this field is OneToOneField instead of normal IntegerField or UUIDField. So when we calling getattr(instance, f.field_name) we are getting parent model instance, but we want to get id. Current code with if checks that this field is pk field and gets _meta.pk.attname that is point to real id.

Example with models from tests:

    media_type = MediaType.objects.create(
        name="some type"
    )
    movie = Movie.objects.create(
        year=2022,
        media_type=media_type,
    )
    Scene.objects.create(
        name="first scene",
        movie=movie,
    )

Now movie has ManyToOneRel with field_name="media_ptr", so in invalidate_m2o getattr(instance, f.field_name) will return Media object. If we want to get id we need to getattr(instance, "media_ptr_id") and i'm getting this string from sender._meta.pk.attname

[aletor123]

@Suor can you please check my description

[Suor]

In situation with multi-table inheritance this field is OneToOneField instead of normal IntegerField or UUIDField. So when we calling getattr(instance, f.field_name) we are getting parent model instance, but we want to get id. Current code with if checks that this field is pk field and gets _meta.pk.attname that is point to real id.

So if we have an fk to a o2o field or another fk this issue will happen too? Which might be done outside if the context of m2m entirely and your fix won't work if that field is not pk, e.g. someone used ForeignKey.to_field.

[aletor123]

Yes, you are right. I've updated implementation

[Suor]

Thanks, I looked into it and it seems correct. I'll give it some more thought, the code doesn't look intuitive so may be missing something here. Although the previous code wasn't intuitive either, simply a bit less complicated )