vijaykatam / django-cache-manager

Cache manager for django models
MIT License
31 stars 19 forks source link

Improper invalidation of child models #26

Open someidiot opened 7 years ago

someidiot commented 7 years ago

This one is confusing me so not sure if I'm using the right terminology here, but I have:

class A(models.Model): stuff

class B(A): stuff

class C(models.Model): a = models.ForeignKey(A)

All three are using CacheManager. When I save an instance of C, it invalidates the instance and all related models - in this case model A. However, if I access B it is using the old cached data. It looks to me like invalidate_model_cache() should invalidate B when it does A.

Does this sound right or am I doing something wrong? If I save any instances of B when I save C it works, but that will be a big pain to do everywhere.

vijaykatam commented 7 years ago

I am assuming you are inheriting B from a concrete model rather than an abstract one? It is generally an anti-pattern to inherit from concrete models, but anyway the answer to your question is that if B does not have any relation(Foreign key, many to many..) other than inheritance then yes it is not going to be invalidated.

I would suggest make model A abstract and then I think things should work as expected.

someidiot commented 7 years ago

A is not abstract (although there is an abstract layer between A and B I didn't show to simplify things). It was a conscious decision not to do so due to the complexities of my app.

In any case, I've added some code in models.py that makes it work for me if you want it (I didn't bother with older Django version support):

def all_subclasses(cls):
    return cls.__subclasses__() + [g for s in cls.__subclasses__()
                                   for g in all_subclasses(s)]

def invalidate_model_cache(sender, instance, **kwargs):
    related_models = set(
        [f.related_model for f in sender._meta.get_fields()
         if f.related_model and (((f.one_to_many or f.one_to_one) and f.auto_created)
         or f.many_to_one or (f.many_to_many and not f.auto_created))])
    for m in related_models.copy():
        related_models.update(all_subclasses(m))
    update_model_cache(sender._meta.db_table)
    for related_model in related_models:
        update_model_cache(related_model._meta.db_table)
vijaykatam commented 7 years ago

The issue with this approach is there could be other classes that are mixed in to a model which are not django models. I understand you may want to keep using inherited models but I can tell you from prior experience that they are a pain to manage when you are making changes to the base concrete model.

At this point I probably don't want to support this since it seems like a one-off, but I am open to considering in the future if more people see the need for it. I will go ahead and update project readme that inheriting from concrete models is not supported currently.

someidiot commented 7 years ago

No worries. I'm still new to this stuff so you're probably right but it's way too late for me to redesign it now :)