Suor / django-cacheops

A slick ORM cache with automatic granular event-driven invalidation.
BSD 3-Clause "New" or "Revised" License
2.09k stars 227 forks source link

Error when trying to cache an abstract model #442

Closed coldcloudgold closed 1 year ago

coldcloudgold commented 1 year ago

Hello!

Found an error when using a bunch of libraries:

When trying to use mass recording (django_pg_bulk_update.bulk_update_or_create), an attempt to cache appears UpdateReturningModel (https://github.com/M1ha-Shvn/django-pg-returning/blob/master/src/django_pg_returning/models.py#L8) model, get its application in the model_profile function (https://github.com/Suor/django-cacheops/blob/master/cacheops/conf.py#L105). This model does not have an application - which causes an error:

Traceback (most recent call last):
   File "/usr/local/lib/python3.9/site-packages/funcy/calc.py", line 59, in wrapper
     return memory[key]
   KeyError: (<class 'django_pg_returning.models.UpdateReturningModel'>,)
   During handling of the above exception, another exception occurred:
   ...
   File "/usr/local/lib/python3.9/site-packages/django_pg_bulk_update/query.py", line 1018, in bulk_update_or_create
     batched_result = batched_operation(batch_func, values,
   File "/usr/local/lib/python3.9/site-packages/django_pg_bulk_update/utils.py", line 175, in batched_operation
     r = handler(*args, **kwargs)
   File "/usr/local/lib/python3.9/site-packages/django_pg_bulk_update/query.py", line 948, in _insert_on_conflict_no_validation
     return _execute_update_query(model, conn, sql, params, ret_fds)
   File "/usr/local/lib/python3.9/site-packages/django_pg_bulk_update/query.py", line 553, in _execute_update_query
     from django_pg_returning import ReturningQuerySet
   File "/usr/local/lib/python3.9/site-packages/django_pg_returning/__init__.py", line 3, in <module>
     from .models import * # noqa F401, F403
   File "/usr/local/lib/python3.9/site-packages/django_pg_returning/models.py", line 8, in <module>
     class UpdateReturningModel(models.Model):
   File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 161, in __new__
     new_class.add_to_class(obj_name, obj)
   File "/usr/local/lib/python3.9/site-packages/django/db/models/base.py", line 326, in add_to_class
     value.contribute_to_class(cls, name)
   File "/usr/local/lib/python3.9/site-packages/cacheops/query.py", line 426, in contribute_to_class
     if cls.__module__ != '__fake__' and family_has_profile(cls):
   File "/usr/local/lib/python3.9/site-packages/funcy/calc.py", line 62, in wrapper
     value = memory[key] = func(*args, **kwargs)
   File "/usr/local/lib/python3.9/site-packages/cacheops/utils.py", line 32, in family_has_profile
     return any(model_profile, model_family(cls))
   File "/usr/local/lib/python3.9/site-packages/funcy/colls.py", line 207, in any
     return _any(xmap(pred, seq))
   File "/usr/local/lib/python3.9/site-packages/cacheops/conf.py", line 105, in model_profile
     app = model._meta.app_label.lower()
AttributeError: 'NoneType' object has no attribute 'lower'

Package versions:

django-pg-returning==2.0.0
django-pg-bulk-update==3.7.0
django-cacheops==6.0
coldcloudgold commented 1 year ago

Hmm, even if you just import the model - an error will appear:

Python 3.9.15 (main, Oct 25 2022, 05:49:37) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.24.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from django_pg_returning.models import UpdateReturningModel
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
/usr/local/lib/python3.9/site-packages/funcy/calc.py in wrapper(*args, **kwargs)
     58             try:
---> 59                 return memory[key]
     60             except KeyError:

KeyError: (<class 'django_pg_returning.models.UpdateReturningModel'>,)

During handling of the above exception, another exception occurred:

AttributeError                            Traceback (most recent call last)
<ipython-input-1-b6fd7938df45> in <module>
----> 1 from django_pg_returning.models import UpdateReturningModel

/usr/local/lib/python3.9/site-packages/django_pg_returning/__init__.py in <module>
      1 from .queryset import *  # noqa F401, F403
      2 from .manager import *  # noqa F401, F403
----> 3 from .models import *  # noqa F401, F403

/usr/local/lib/python3.9/site-packages/django_pg_returning/models.py in <module>
      6 
      7 
----> 8 class UpdateReturningModel(models.Model):
      9     class Meta:
     10         abstract = True

/usr/local/lib/python3.9/site-packages/django/db/models/base.py in __new__(cls, name, bases, attrs, **kwargs)
    159         # to the class.
    160         for obj_name, obj in contributable_attrs.items():
--> 161             new_class.add_to_class(obj_name, obj)
    162 
    163         # All the fields of any type declared on this model

/usr/local/lib/python3.9/site-packages/django/db/models/base.py in add_to_class(cls, name, value)
    324     def add_to_class(cls, name, value):
    325         if _has_contribute_to_class(value):
--> 326             value.contribute_to_class(cls, name)
    327         else:
    328             setattr(cls, name, value)

/usr/local/lib/python3.9/site-packages/cacheops/query.py in contribute_to_class(self, cls, name)
    424         # NOTE: we make it here rather then inside _install_cacheops()
    425         #       because we don't want @once_per() to hold refs to all of them.
--> 426         if cls.__module__ != '__fake__' and family_has_profile(cls):
    427             self._install_cacheops(cls)
    428 

/usr/local/lib/python3.9/site-packages/funcy/calc.py in wrapper(*args, **kwargs)
     60             except KeyError:
     61                 try:
---> 62                     value = memory[key] = func(*args, **kwargs)
     63                     return value
     64                 except SkipMemoization as e:

/usr/local/lib/python3.9/site-packages/cacheops/utils.py in family_has_profile(cls)
     30 @memoize
     31 def family_has_profile(cls):
---> 32     return any(model_profile, model_family(cls))
     33 
     34 

/usr/local/lib/python3.9/site-packages/funcy/colls.py in any(pred, seq)
    205     if seq is EMPTY:
    206         return _any(pred)
--> 207     return _any(xmap(pred, seq))
    208 
    209 def none(pred, seq=EMPTY):

/usr/local/lib/python3.9/site-packages/cacheops/conf.py in model_profile(model)
    103     model_profiles = prepare_profiles()
    104 
--> 105     app = model._meta.app_label.lower()
    106     model_name = model._meta.model_name
    107     for guess in ('%s.%s' % (app, model_name), '%s.*' % app, '*.*'):

AttributeError: 'NoneType' object has no attribute 'lower'
Suor commented 1 year ago

Can you reduce this to a test? Starting from checking whether all of those deps are required to reproduce. Ideally it would be a test for cacheops but any way to reproduce it will be helpful.

coldcloudgold commented 1 year ago

Can you reduce this to a test? Starting from checking whether all of those deps are required to reproduce. Ideally it would be a test for cacheops but any way to reproduce it will be helpful.

For convenience, I created an empty django project with the necessary environment and test. You can check out the link - https://github.com/coldcloudgold/Demo_repo_cacheops

Suor commented 1 year ago

Looking into it more, it is really weird that UpdateReturningModel._meta.app_label is None, even for an abstract model. Is that even allowed or is that a bug? Any thoughts @M1ha-Shvn?

I don't mind shielding against that though in model_profile().

M1ha-Shvn commented 1 year ago

I think that UpdateReturningModel._meta.app_label is None as django_pg_returning is not in django's INSTALLED_APPS settings parameter. My library is just a python package which is not supposed to be installed as app. UpdateReturningModel is expected to be inherited in some django app and never be used directly. And the inherited model, I suppose, will contain this property. So, here I think it's reasonable checking if model has app and skip patching if not.

Suor commented 1 year ago

I still don't quite get why model_profile() gets called on an abstract model. It looks like this should not happen, so simply skipping it will fix the symptom, which might be or not be enough.

Suor commented 1 year ago

Ok, tracking this it looks like model_family() and family_has_profile() should work differently. Currently the latter may return True for an abstract model, which leads to installing signals for it. These are never sent so this does not lead to a wrong behavior though.

Anyways the correct fix would be to shoot it early, before it reaches the model_profile().