Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle actions by fk related manager #146

Open
jerch opened this issue Feb 17, 2024 · 1 comment
Open

handle actions by fk related manager #146

jerch opened this issue Feb 17, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@jerch
Copy link
Collaborator

jerch commented Feb 17, 2024

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 jerch added the bug Something isn't working label Feb 17, 2024
@jerch
Copy link
Collaborator Author

jerch commented Feb 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant