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

feat(m2m-fields): allow to pass custom through M2M #1149

Conversation

antoineauger
Copy link

Description

This MR is an attempt for allowing to pass a M2M model (possibly defined with through/through_fields attributes) to the m2m_fields.
The ManyToManyField model can be defined with through being either a string or a model instance. The through_fields can be a tuple of strings.

Related Issue

Related #1094

Related #1093

Motivation and Context

This MR is intended to allow the following to work:

class PollWithManyToManyThroughFields(models.Model):
    question = models.CharField(max_length=200)
    people = models.ManyToManyField("Place", through="Venue", through_fields=("place", "guest"))

    history = HistoricalRecords(m2m_fields=[people])

class Venue(models.Model):
    place = models.ForeignKey("Place", on_delete=models.CASCADE, null=True)
    guest = models.ForeignKey("Person", on_delete=models.CASCADE, null=True)

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run the pre-commit run command to format and lint.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added my name and/or github handle to AUTHORS.rst
  • I have added my change to CHANGES.rst
  • All new and existing tests passed.

@antoineauger antoineauger marked this pull request as draft March 27, 2023 15:37
@antoineauger antoineauger force-pushed the feat/custom-m2m-fields-strings branch from 5a11001 to 8399236 Compare April 3, 2023 12:27
@antoineauger antoineauger force-pushed the feat/custom-m2m-fields-strings branch from 8399236 to 5b42804 Compare April 20, 2023 12:36
@antoineauger
Copy link
Author

@legau (or someone else - I just saw that you commented on a related MR so you might have clues) I would need some help here in order to progress with that MR. 🙇🏻

I started by writing the tests that should pass. I'm now trying to see where the problem(s) is/are with the current implementation.

When I try to run tests for this MR, I get the following error:

ValueError: Invalid model reference 'Venue'. String model references must be of the form 'app_label.ModelName'.
Stacktrace
$ python3 runtests.py


Traceback (most recent call last):
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/utils.py", line 15, in make_model_tuple
    app_label, model_name = model.split(".")
ValueError: not enough values to unpack (expected 2, got 1)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/antoine/_Github/django-simple-history/runtests.py", line 173, in <module>
    main()
  File "/home/antoine/_Github/django-simple-history/runtests.py", line 160, in main
    django.setup()
  File "/home/antoine/.local/lib/python3.10/site-packages/django/__init__.py", line 24, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/home/antoine/.local/lib/python3.10/site-packages/django/apps/registry.py", line 116, in populate
    app_config.import_models()
  File "/home/antoine/.local/lib/python3.10/site-packages/django/apps/config.py", line 269, in import_models
    self.models_module = import_module(models_module_name)
  File "/usr/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/home/antoine/_Github/django-simple-history/simple_history/tests/models.py", line 136, in <module>
    class PollWithManyToManyThroughFields(models.Model):
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/base.py", line 363, in __new__
    new_class._prepare()
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/base.py", line 426, in _prepare
    class_prepared.send(sender=cls)
  File "/home/antoine/.local/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 176, in send
    return [
  File "/home/antoine/.local/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 177, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/home/antoine/_Github/django-simple-history/simple_history/models.py", line 212, in finalize
    m2m_changed.connect(
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/signals.py", line 27, in connect
    self._lazy_method(
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/signals.py", line 22, in _lazy_method
    apps.lazy_model_operation(partial_method, make_model_tuple(sender))
  File "/home/antoine/.local/lib/python3.10/site-packages/django/db/models/utils.py", line 22, in make_model_tuple
    raise ValueError(
ValueError: Invalid model reference 'Venue'. String model references must be of the form 'app_label.ModelName'.

Looking at other models/tests that pass, for instance PollWithManyToManyCustomHistoryID, I would say that this this resolution is not automatically done by django (or django-simple-history) when through/through_fields params are present.

While I understand that I need to resolve this in an app_label.ModelName form, I cannot figure out how to perform this while all Django models are not initialized and a lot of relations are resolved later in a lazy fashion 🤔

I would appreciate any hint here, and would be happy to contribute to this great library 👌🏻

@legau
Copy link
Contributor

legau commented Apr 21, 2023

The m2m_changed signal tries to access the Venue model which is not yet initialized (lazy string notation). You have to find a way to make it connect at a later time, you could do some hacky stuff like injecting an override of the ManyToManyField.contribute_to_class of your field but I'm not sure if that's the best approach

@lsmith77
Copy link

any chance to get this PR completed?

@antoineauger
Copy link
Author

any chance to get this PR completed?

@lsmith77 I kinda lack of Django knowledge regarding signals/lazy fields on my side to make any progress here, feel free to pick it up if you have an idea how to implement this 🙇🏻

@lsmith77
Copy link

a bummer .. I am a total Django newbie (and also still a Python beginner).

@antoineauger antoineauger force-pushed the feat/custom-m2m-fields-strings branch from a0c2739 to 10809f1 Compare September 28, 2023 13:57
@antoineauger
Copy link
Author

I couldn't find a way to make this work, I'll create an issue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants