django-computedfields icon indicating copy to clipboard operation
django-computedfields copied to clipboard

handle actions by fk related manager

Open jerch opened this issue 1 year ago • 1 comments

Currently many actions of related managers are not properly handled - some work, others dont. Seems the reason for some not working comes from bulk usage internally (all have a bulk=True argument).

Not sure yet, if and how we can get this working for all methods, main issue here is the dynamic creation of the managers and whether we can intercept/overload that somehow. If thats not possible, we should at least put a warning in the docs.

This is also linked to proper M2M handling, as it is done similar by django (but other than fk related manager actions, M2M actions are currently covered by signal handlers in CF). If we find a better way to deal with the mass actions from related managers in general we prolly should revisit the M2M handlers as well.

Ref: https://docs.djangoproject.com/en/5.0/ref/models/relations/

jerch avatar Feb 17 '24 18:02 jerch

For ref (dont like yet, where this leads) - quick&dirty test to get a hold of the related manager creation:

    def _patch_fks(self) -> None:
        from django.utils.functional import cached_property
        from django.db.models.fields.related_descriptors import create_reverse_many_to_one_manager

        def injected(self):
            related_model = self.rel.related_model
            print('--------------- injected! ------------------')
            return create_reverse_many_to_one_manager(
                related_model._default_manager.__class__,
                self.rel,
            )

        if self.get_contributing_fks():
            from django.db.models.fields.related_descriptors import ReverseManyToOneDescriptor
            ReverseManyToOneDescriptor.related_manager_cls = cached_property(injected)
            ReverseManyToOneDescriptor.related_manager_cls.__set_name__(ReverseManyToOneDescriptor, 'related_manager_cls')

With this it is possible to provide our own create_reverse_many_to_one_manager impl returning a custom RelatedManager with cf update logic for contributing fk fields even in the bulk case.

But before going down the monkey patching rabbit hole, there are several things to clarify:

  • First identify/test faulty manager actions for all relation types (fk, o2o, m2m).
  • If we introduce automatic bulk action handling for related managers, then what about bulk actions on model managers in the first place? Should those also be addressed? While that would make default usage of CF a breeze with single shot bulk actions, it is really hard to get properly automated for multiple consecutive bulk actions, thus might be a futile endeavour.
  • Monkey patching django at this level looks quite wrong, is there any better or official way to use a custom RelatedManager w'o overloading half of the bootstrapping?
  • Are there circumstances, where fks dont rely on related managers created from ReverseManyToOneDescriptor for backward actions? Other custom overloads? Do we need to inspect every single contributing fk field here?
  • Can we adopt this for better M2M handling? This also puts the current signal handlers on trial.

jerch avatar Feb 18 '24 12:02 jerch