Opened 14 years ago

Last modified 4 years ago

#12203 new Bug

ManyToManyField with through model can't be used in admin

Reported by: David Gouldin Owned by: nobody
Component: contrib.admin Version: dev
Severity: Normal Keywords: M2M, admin, through, through_fields
Cc: dgouldin@…, glicerinu@…, cmawebsite@…, frnhr Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Using the following models:

class Author(models.Model):
    name = models.CharField(max_length=100)

class Book(models.Model):
    name = models.CharField(max_length=100)
    authors = models.ManyToManyField(Author, through='AuthorsBooks')

class AuthorsBooks(models.Model):
    author = models.ForeignKey(Author)
    book = models.ForeignKey(Book)

... with this admin:

class BookAdmin(admin.ModelAdmin):
    fields = ['authors']

... results in a TemplateSyntaxError. Removing the through kwarg in Book.author field definition, the same admin class works.

Attachments (4)

12203-m2m-w-through-modeladmin-fields-option1.diff (2.4 KB ) - added by Ramiro Morales 14 years ago.
patch implementing first admin validation strategy described by Russell
12203-m2m-w-through-modeladmin-fields-option1.2.diff (2.4 KB ) - added by Ramiro Morales 14 years ago.
Updated patch for option 1 to take in account the fact that all m2m rels have through now. Thanks Alex Gaynor.
12203-m2m-w-through-modeladmin-fields-option2.diff (7.0 KB ) - added by Ramiro Morales 14 years ago.
Patch implementing fancy admin validation strategy described by Russell.
12203-m2m-w-through-modeladmin-fields-option1.3.diff (1.6 KB ) - added by VINAY KUMAR SHARMA 10 years ago.
do not report error if M2M field has through_fields defined

Download all attachments as: .zip

Change History (27)

comment:1 by David Gouldin, 14 years ago

Cc: dgouldin@… added

comment:2 by Ramiro Morales, 14 years ago

I don't know about the exact TemplateSyntaxError exception being thrown, but the fact that no widget can be used is sensible and documented behavior, with a documented alternative that involves inlines:

citing:

"...when you specify an intermediary model using the through argument to a ManyToManyField, the admin will not display a widget by default. This is because each instance of that intermediary model requires more information than could be displayed in a single widget, and the layout required for multiple widgets will vary depending on the intermediate model."

comment:3 by Johannes Dollinger, 14 years ago

Possibly related: #9475

comment:4 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

Ramiro has picked the problem correctly - this is a case of doing something you shouldn't. If you don't have a fields specifier on the ModelAdmin, you won't get an m2m widget because of the explicit m2m through model. Explicitly specifying one should be an error. However, the error handling for this case could certainly be improved.

The simple solution is to raise an ImproperlyConfigured error if an m2m with an explicit through model is specified in the fields list.

The fancy solution would be to loosen this condition slightly and allow an m2m field to be in the fields list (and therefore allow an m2m widget be used) *iff* the through model is just 2 foreign keys with a unique_together pairing (i.e., if the through model is exactly the same as the m2m model that is be automatically generated for the non-explicit through case).

Longer term, when a solution to #9475 is worked out, it should be possible loosen the condition even further, and allow the use of a the default m2m widget for cases where the m2m through model can be saved without any additional details.

comment:5 by David Gouldin, 14 years ago

I understand the difficulty the added complexity of extra m2m fields creates. In my case, I simply want to port a non-django db schema to django models, and I don't know any other way to create a m2m relation other than using through. That seemed to me the intent of #6095: to be able to specify your own model for m2m and use it just as you'd use a regular m2m model field. Unless you can point me in an alternate direction for legacy schema porting, I think it's a reasonable request to be able to use the simplest case m2m through model just as a regular django m2m.

comment:6 by Russell Keith-Magee, 14 years ago

The original intent of #6095 was to allow for data to be stored as part of the m2m relation - for example, in a 'Membership" relation between "Person" and "Group", you might want to store the level of membership. The ability to support legacy m2m tables was a bonus, since it's the redundant case of an m2m relation without any intermediate data.

I completely agree that this is a reasonable request - it's just not a trivial request. If I wasn't in favour of the request, I would have closed the ticket; instead, I marked it accepted. That doesn't mean I'm personally going to work on it any time soon, but if someone were to develop a solution, it would be a candidate for trunk.

For the benefit of anyone chasing this ticket - make sure you check all the edge cases. For example, the model described in the ticket actually isn't strictly analogous with Django's m2m, because it doesn't enforce uniqueness between author and book.

by Ramiro Morales, 14 years ago

patch implementing first admin validation strategy described by Russell

by Ramiro Morales, 14 years ago

Updated patch for option 1 to take in account the fact that all m2m rels have through now. Thanks Alex Gaynor.

by Ramiro Morales, 14 years ago

Patch implementing fancy admin validation strategy described by Russell.

comment:7 by Ramiro Morales, 14 years ago

Has patch: set
Needs documentation: set

I've attached first iterations of patches implementing the two strategies outlined by Russell.

In the option2 one I've tried to cover all the corner cases. Something I'm not sure is the through-using, non-symmetrical, recursive m2m case. Can we allow the admin add/change forms to show the plain widget?.

Also, for both patches the exact text shown next to the ImproperlyConfigured exception are open to be enhanced.

I will add documentation changes in (s) future interation(s).

See also tickets #10010 and #11126.

Replying to dgouldin:

to be able to specify your own model for m2m and use it just as you'd use a regular m2m model field. Unless you can point me in an alternate direction for legacy schema porting, I think it's a reasonable request to be able to use the simplest case m2m through model just as a regular django m2m.

If your simplest legacy intermediate table complies with the Django m2m restrictions/limitations: e.g. it has just one PK then one possible alternative could be to use the db_table ManyToManyField field option (http://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.ManyToManyField.db_table) and don't use a explicit through model.

comment:8 by Russell Keith-Magee, 14 years ago

(In [11737]) Refs #12203 -- Improved error handling for the case where a user manually specifies an m2m field with an explicit through field. Thanks to dgouldin for the report, and Ramiro Morales for the patch.

comment:9 by Russell Keith-Magee, 14 years ago

Has patch: unset
Needs documentation: unset

I've committed Ramiro's patch that improves error handling, but I'll leave the ticket open as a placeholder for the feature request of allowing m2m fields if the manually specified model is compatible with Django's requirements.

comment:10 by Marc Aymerich, 13 years ago

Cc: glicerinu@… added

comment:11 by Matt McClanahan, 13 years ago

Severity: Normal
Type: New feature

comment:12 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:13 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

by VINAY KUMAR SHARMA, 10 years ago

do not report error if M2M field has through_fields defined

comment:14 by VINAY KUMAR SHARMA, 10 years ago

Has patch: set
Keywords: M2M admin through through_fields added
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Resolution: fixed
Status: newclosed
Type: New featureBug
Version: 1.1master

Have fixed!

Do not add any error if M2M field has through_fields and through model is not auto_created!

Attachment: 12203-m2m-w-through-modeladmin-fields-option1.3.diff

comment:15 by Tim Graham, 10 years ago

Resolution: fixed
Status: closednew

A ticket isn't closed until the fix is committed. Also, your patch would need a test.

comment:16 by Tim Graham, 10 years ago

Needs documentation: unset
Needs tests: unset

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

Last edited 10 years ago by Tim Graham (previous) (diff)

comment:17 by Collin Anderson, 9 years ago

Cc: cmawebsite@… added
Summary: ManyToManyField with explicit through model can't be used as an admin fieldManyToManyField with through model can't be used in admin

We need to fix #6707 first, otherwise it will revert to the default extra fields every time you hit save.

comment:18 by Tim Graham, 8 years ago

See also #26998 for how this applies to django-taggit.

comment:19 by Dmitry Mugtasimov, 4 years ago

If you also waiting for the fix and have a simple through model for some reason you may use auto_created = True to trick Django Admin:

class JobTitleExperienceThrough(models.Model):
    title = models.ForeignKey('JobTitle', on_delete=models.CASCADE,
                              related_name='experiences_through')
    experience = models.ForeignKey('Experience', on_delete=models.CASCADE,
                                   related_name='titles_trough')

    class Meta:
        # TODO(dmu) MEDIUM: Remove `auto_created = True` after these issues are fixed:
        #                   https://code.djangoproject.com/ticket/12203 and
        #                   https://github.com/django/django/pull/10829
        auto_created = True


class JobTitle(models.Model):
    name = models.CharField(max_length=64)
    role = models.ForeignKey('JobRole', on_delete=models.CASCADE,
                             related_name='titles')
    experiences = models.ManyToManyField('Experience', through='JobTitleExperienceThrough',
                                         related_name='titles', blank=True)

    class Meta:
        ordering = ('id',)

comment:20 by Sylvain Fankhauser, 4 years ago

Unfortunately, the auto_created=True hack doesn't work if you want to use foreign keys to the through model. You'll get a (fields.E300) Field defines a relation with model 'JobTitleExperienceThrough', which is either not installed, or is abstract.

comment:21 by frnhr, 4 years ago

Cc: frnhr added

comment:22 by Dmitry Mugtasimov, 4 years ago

For some reason I ended up with this code:

class JobTitleAdmin(admin.ModelAdmin):
    def formfield_for_manytomany(self, *args, **kwargs):  # pylint: disable=arguments-differ
        # TODO(dmu) MEDIUM: Remove `auto_created = True` after these issues are fixed:
        #                   https://code.djangoproject.com/ticket/12203 and
        #                   https://github.com/django/django/pull/10829

        # We trick Django here to avoid `./manage.py makemigrations` produce unneeded migrations
        JobTitleExperienceThrough._meta.auto_created = True  # pylint: disable=protected-access
        return super().formfield_for_manytomany(*args, **kwargs)


class JobTitle(models.Model):
    name = models.CharField(max_length=64)
    role = models.ForeignKey('JobRole', on_delete=models.CASCADE,
                             related_name='titles')
    experiences = models.ManyToManyField('Experience', through='JobTitleExperienceThrough',
                                         related_name='titles', blank=True)

    class Meta:
        ordering = ('id',)

in reply to:  22 comment:23 by dennisvang, 4 years ago

Replying to Dmitry Mugtasimov:

The trick of setting JobTitleExperienceThrough._meta.auto_created = True in formfield_for_manytomany does indeed enable the ModelMultipleChoiceField on the admin page without causing migration issues (as far as I can see).

However, there is a dangerous side-effect of using this, in case you have other models with a "cascading" relation directly to your explicit through-model, e.g. models.ForeignKey(to=JobTitleExperienceThrough, on_delete=models.CASCADE):

If the initial queryset for the m2m field JobTitle.experiences is filtered (e.g. as in the docs https://docs.djangoproject.com/en/3.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.formfield_for_manytomany), and, for whatever reason, the filter excludes some Experience objects that have previously been assigned, then the corresponding records from JobTitleExperienceThrough will be silently deleted, including any records for other models pointing to them.

This means there is a serious potential for "silent" data loss.

This is because the many-to-many relation will now be updated using ManyRelatedManager.set() (via ModelAdmin.save_related() -> form.save_m2m() -> BaseModelForm._save_m2m() -> ManyToManyField.save_form_data()).

Silent deletion could be prevented by setting on_delete=models.PROTECT on any relation to the explicit through-model, but then you would first have to be aware of the necessity.

When using a "real" auto-created through table (i.e. an implicit one) the issue does not arise, because there is no way to set a ForeignKey to the implicit through, as far as I know.

When using an inline for JobTitleExperienceThrough, this issue does not occur.

Note: See TracTickets for help on using tickets.
Back to Top